Skip to content

KAFKA-8407: Fix validation of class and list configs in connector client overrides#6789

Merged
rhauch merged 1 commit into
apache:trunkfrom
C0urante:kafka-8407
May 23, 2019
Merged

KAFKA-8407: Fix validation of class and list configs in connector client overrides#6789
rhauch merged 1 commit into
apache:trunkfrom
C0urante:kafka-8407

Conversation

@C0urante

@C0urante C0urante commented May 22, 2019

Copy link
Copy Markdown
Contributor

Jira

Because of how config values are converted into strings in the AbstractHerder.validateClientOverrides() method after being validated by the client override policy, an exception is thrown if the value returned by the policy isn't already parsed as the type expected by the client ConfigDef. A more thorough writeup of how this happens is available in the linked Jira ticket.

The fix here involves parsing client override properties before passing them to the override policy.

A unit test is added to ensure that several different types of configs are validated properly by the herder.

This bug fix should be included in the recently-cut 2.3 branch.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@C0urante

Copy link
Copy Markdown
Contributor Author

@mageshn, since this was introduced by KIP-458 would you mind taking a look? :)

@mageshn mageshn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @C0urante. LGTM.

@C0urante

Copy link
Copy Markdown
Contributor Author

@rhauch, could you take a look at this when you have a moment?

@rhauch rhauch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @C0urante. One question below.

@rhauch rhauch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @C0urante!

@rhauch
rhauch merged commit 5351efe into apache:trunk May 23, 2019
rhauch pushed a commit that referenced this pull request May 23, 2019
…ent overrides (#6789)

Because of how config values are converted into strings in the `AbstractHerder.validateClientOverrides()` method after being validated by the client override policy, an exception is thrown if the value returned by the policy isn't already parsed as the type expected by the client `ConfigDef`. The fix here involves parsing client override properties before passing them to the override policy.

A unit test is added to ensure that several different types of configs are validated properly by the herder.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Magesh Nandakumar <magesh.n.kumar@gmail.com>, Randall Hauch <rhauch@gmail.com>
@C0urante
C0urante deleted the kafka-8407 branch May 23, 2019 21:22
haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
…ent overrides (apache#6789)

Because of how config values are converted into strings in the `AbstractHerder.validateClientOverrides()` method after being validated by the client override policy, an exception is thrown if the value returned by the policy isn't already parsed as the type expected by the client `ConfigDef`. The fix here involves parsing client override properties before passing them to the override policy.

A unit test is added to ensure that several different types of configs are validated properly by the herder.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Magesh Nandakumar <magesh.n.kumar@gmail.com>, Randall Hauch <rhauch@gmail.com>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…ent overrides (apache#6789)

Because of how config values are converted into strings in the `AbstractHerder.validateClientOverrides()` method after being validated by the client override policy, an exception is thrown if the value returned by the policy isn't already parsed as the type expected by the client `ConfigDef`. The fix here involves parsing client override properties before passing them to the override policy.

A unit test is added to ensure that several different types of configs are validated properly by the herder.

Author: Chris Egerton <chrise@confluent.io>
Reviewers: Magesh Nandakumar <magesh.n.kumar@gmail.com>, Randall Hauch <rhauch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants