lucasbru merged PR #14680:
URL: https://github.com/apache/kafka/pull/14680
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache
lucasbru commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1818426591
Merged trunk. Test failures are unrelated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1817393641
Thanks @lucasbru
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
lucasbru commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1815926638
Scala compilation in 2.12 is broken on master, which is fixed in
https://github.com/apache/kafka/pull/14786, otherwise this is looking good
--
This is an automated message from the Apa
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1815737836
hi @lucasbru - I think the KafkaConsumerTest failures were caused by some
test we forgot to disabled. The last 2 runs seem to be ok (well, still flaky
but not failing on that specific
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1815186022
The most recent build passed - let me re-trigger the tests to make sure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1813871310
@lucasbru - I think we probably need to refactor KafkaConsumerTest again
because most of the tests don't really apply to the async KafkaConsumer due to
different protocol use etc.
-
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1813848500
I think the test failures was probably caused by heartbeat bubbled up to the
consumer subsequently throwing an exception
--
This is an automated message from the Apache Git Service.
T
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1813394707
I rebased the trunk and was unable to reproduce the failure locally. I
wonder if the tests is flaky and was caused by the delegate PR.
--
This is an automated message from the Apache
philipnee opened a new pull request, #14680:
URL: https://github.com/apache/kafka/pull/14680
The PR covers a few important points:
1. Exception handling: We should be thrown RetriableCommitException when the
commit exception is retriable. We should throw FencedIdException on commit and
philipnee closed pull request #14680: KAFKA-15174: Ensure CommitAsync propagate
the exception to the user
URL: https://github.com/apache/kafka/pull/14680
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1813338012
hm - why is it sending heartbeat? looking
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to
lucasbru commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1813330936
@philipnee there is a suspicious test failure for
`org.apache.kafka.clients.consumer.KafkaConsumerTest`, could you have a look?
--
This is an automated message from the Apache Git Serv
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1813009735
Hi @lucasbru - Addressed your comment about the volatile. I also modify the
invoker.submit to catch some java exception. Please review. Terrible sorry
but I needed to rebase to the c
lucasbru commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1394089557
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -147,6 +151,10 @@ public class PrototypeAsyncConsumer implement
lucasbru commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1812360189
Okay. Sounds like using the background events queue wouldn't strictly have
downsides, but also not have so many upsides. I agree that picking things from
a queue sounds awkward, so let's
kirktrue commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1811620894
@lucasbru As @philipnee said, the two mechanisms appear similar and do have
overlap. However, not only is it "awkward" to mix them, but it raises
correctness questions.
`poll()` p
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1810775229
Hi @lucasbru - Yes. It is not quite feasible to use the backgroundEventQueue
because of the behavior of the current API, i.e., the callback needs to be
invoked ouside of the poll call.
lucasbru commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1809959390
Hi @philipnee. Looks good, I think all my comments got addressed. I see a
comment from @kirktrue about using the background event queue instead of a
separate invoker queue, did we resolv
lucasbru commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1392317044
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -147,6 +151,10 @@ public class PrototypeAsyncConsumer implement
lucasbru commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1392314116
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -712,6 +729,10 @@ private void close(Duration timeout, boolean
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1809546161
Hi @lucasbru - Thanks for your feedback, the PR was updated. Would you have
time to go over the comments again? Thanks!
--
This is an automated message from the Apache Git Service.
T
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388420510
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -712,6 +729,10 @@ private void close(Duration timeout, boolean
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388411256
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -1096,4 +,76 @@ private void subscribeInternal(Collection
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388410485
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -1096,4 +,76 @@ private void subscribeInternal(Collection
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388409913
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -381,21 +391,28 @@ public void commitAsync(OffsetCommitCallbac
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388405248
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -381,21 +391,28 @@ public void commitAsync(OffsetCommitCallbac
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388398462
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -147,6 +151,10 @@ public class PrototypeAsyncConsumer implemen
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388387504
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -1096,4 +,76 @@ private void subscribeInternal(Collection
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388377068
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -1010,15 +1031,9 @@ private void updateLastSeenEpochIfNewer(To
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1388374351
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -1010,15 +1031,9 @@ private void updateLastSeenEpochIfNewer(To
lucasbru commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1387628672
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -278,7 +286,8 @@ public PrototypeAsyncConsumer(final Time time,
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1386995116
##
clients/src/test/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumerTest.java:
##
@@ -168,6 +200,45 @@ public void testCommitted_ExceptionThrow
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1802364240
Hi @kirktrue Thanks for taking time reviewing the PR: Just to quickly
respond to your comment.
The BackgroundEventProcessor is already invoked inside poll() and if the
event is a
kirktrue commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1380596565
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -381,20 +394,33 @@ public void commitAsync(OffsetCommitCallback
philipnee commented on PR #14680:
URL: https://github.com/apache/kafka/pull/14680#issuecomment-1791913330
Hi @dajac @kirktrue - Thanks for offering valuable feedbacks to this PR,
much appreciated. I wonder how do you feel about using a local queue to stash
the new callbacks? The goal really
philipnee commented on code in PR #14680:
URL: https://github.com/apache/kafka/pull/14680#discussion_r1380850627
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/PrototypeAsyncConsumer.java:
##
@@ -1096,4 +1114,47 @@ private void subscribeInternal(Collection
37 matches
Mail list logo
