-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-7620: Fix restart logic for TTLs in WorkerConfigTransformer #5914
Conversation
@ewencp , could I kindly get a review? Thanks in advance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @rayokota. This looks good to me. I had a couple of minor questions/comments.
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfigTransformer.java
Outdated
Show resolved
Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java
Show resolved
Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java
Show resolved
Hide resolved
Thanks @mageshn ! I've incorporated your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks close, just a couple small comments.
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfigTransformer.java
Outdated
Show resolved
Hide resolved
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerConfigTransformer.java
Show resolved
Hide resolved
connect/runtime/src/test/java/org/apache/kafka/connect/runtime/WorkerConfigTransformerTest.java
Outdated
Show resolved
Hide resolved
@ewencp , thanks for the review! I've incorporated your feedback. Let me know if there are any other changes you would like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix. Best part is that this turned into a net negative LOC change in a bug fix :) Merging back through 2.0 branch
The restart logic for TTLs in `WorkerConfigTransformer` was broken when trying to make it toggle-able. Accessing the toggle through the `Herder` causes the same code to be called recursively. This fix just accesses the toggle by simply looking in the properties map that is passed to `WorkerConfigTransformer`. Author: Robert Yokota <rayokota@gmail.com> Reviewers: Magesh Nandakumar <magesh.n.kumar@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes #5914 from rayokota/KAFKA-7620 (cherry picked from commit a2e87fe) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
The restart logic for TTLs in `WorkerConfigTransformer` was broken when trying to make it toggle-able. Accessing the toggle through the `Herder` causes the same code to be called recursively. This fix just accesses the toggle by simply looking in the properties map that is passed to `WorkerConfigTransformer`. Author: Robert Yokota <rayokota@gmail.com> Reviewers: Magesh Nandakumar <magesh.n.kumar@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes #5914 from rayokota/KAFKA-7620 (cherry picked from commit a2e87fe) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
The restart logic for TTLs in `WorkerConfigTransformer` was broken when trying to make it toggle-able. Accessing the toggle through the `Herder` causes the same code to be called recursively. This fix just accesses the toggle by simply looking in the properties map that is passed to `WorkerConfigTransformer`. Author: Robert Yokota <rayokota@gmail.com> Reviewers: Magesh Nandakumar <magesh.n.kumar@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#5914 from rayokota/KAFKA-7620
The restart logic for TTLs in
WorkerConfigTransformer
was broken when trying to make it toggle-able. Accessing the toggle through theHerder
causes the same code to be called recursively. This fix just accesses the toggle by simply looking in the properties map that is passed toWorkerConfigTransformer
.Committer Checklist (excluded from commit message)