-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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-14247: add handler impl to the prototype #12775
KAFKA-14247: add handler impl to the prototype #12775
Conversation
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 @philipnee!
I assume the bulk of the additions here are copy-and-pasted from the existing KafkaConsumer
?
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java
Show resolved
Hide resolved
…als/PrototypeAsyncConsumer.java Co-authored-by: Kirk True <kirk@kirktrue.pro>
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 for the PR, @philipnee !
I had a couple of small suggestions.
Do you think it's possible to also include a quick test to demonstrate that the code in this PR is effective?
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java
Outdated
Show resolved
Hide resolved
Added a test for the subscription() method. It's a bit complicated to test other method as the class is still Abstract. |
@@ -64,27 +64,6 @@ public class DefaultBackgroundThread extends KafkaThread { | |||
private final AtomicReference<Optional<RuntimeException>> exception = | |||
new AtomicReference<>(Optional.empty()); | |||
|
|||
public DefaultBackgroundThread(final ConsumerConfig config, |
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.
removing unused constructor for now.
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, @philipnee .
I don't see how it could be related, but Gradle got a bunch of OOMEs on your last test run, so I kicked it off again. |
All right, thanks for that fix, @philipnee ! This time, the failures look unrelated:
I'll go ahead and merge. |
Minor revision for KAFKA-14247. Added how the handler is called and constructed to the prototype code path. Reviewers: John Roesler <vvcephei@apache.org>, Kirk True <kirk@mustardgrain.com>
Minor revision for KAFKA-14247. Added how the handler is called and constructed to the prototype code path. Reviewers: John Roesler <vvcephei@apache.org>, Kirk True <kirk@mustardgrain.com>
Minor revision for KAFKA-14247. Added how the handler is called and constructed to the prototype code path. Reviewers: John Roesler <vvcephei@apache.org>, Kirk True <kirk@mustardgrain.com> Co-authored-by: Philip Nee <pnee@confluent.io>
Minor revision for KAFKA-14247. Added how the handler is called and constructed to the prototype code path. Reviewers: John Roesler <vvcephei@apache.org>, Kirk True <kirk@mustardgrain.com> Co-authored-by: Philip Nee <pnee@confluent.io>
*This is a minor revision for KAFKA-14247. We added how the handler is called and constructed to the prototype code path.
I also added the SubscriptionState instance to elucidate the idea that it is created on the client thread. The
subscription()
demonstrates that it maintains the same access pattern as the current impl.Committer Checklist (excluded from commit message)