-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-8421: Still return data during rebalance #7312
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
Changes from 1 commit
37b5c63
4a34116
d964cea
f5b8f96
b07bac2
29f8942
3000d6f
8464fa7
f804b3d
3c62029
284ef77
9825063
929048a
58adc09
c79a93d
75c4e88
95ef3fb
3b844ff
781514d
8b08a3e
8fa5d74
d75fcb9
b176e4e
7a826f9
31e372e
366739b
faf787e
c71af82
ffe6146
518049c
30d5e53
fa3456d
0220d96
27d013a
94611cc
b529847
33fd785
1df9808
3057405
21f531d
effb29d
6fbd976
f4e7111
91b6606
0ad52ec
27f76b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,8 +22,22 @@ public class RetriableCommitFailedException extends RetriableException { | |
|
|
||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| public static RetriableCommitFailedException withUnderlyingMessage(String additionalMessage) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok removing this API in spite of the compatibility concern. It's just that the other constructors are "standard" exception constructors and we have no real need to remove them. |
||
| return new RetriableCommitFailedException("Offset commit failed with a retriable exception. " + | ||
| "You should retry committing the latest consumed offsets. " + | ||
| "The underlying error was: " + additionalMessage); | ||
| } | ||
|
|
||
| public RetriableCommitFailedException(Throwable t) { | ||
| super("Offset commit failed with a retriable exception. You should retry committing " + | ||
| "the latest consumed offsets.", t); | ||
| } | ||
|
|
||
| public RetriableCommitFailedException(String message) { | ||
| super(message); | ||
| } | ||
|
|
||
| public RetriableCommitFailedException(String message, Throwable t) { | ||
| super(message, t); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,7 +131,7 @@ protected enum MemberState { | |
| private HeartbeatThread heartbeatThread = null; | ||
| private RequestFuture<ByteBuffer> joinFuture = null; | ||
| private RequestFuture<Void> findCoordinatorFuture = null; | ||
| private RuntimeException findCoordinatorException = null; | ||
| volatile private RuntimeException findCoordinatorException = null; | ||
| private Generation generation = Generation.NO_GENERATION; | ||
| private long lastRebalanceStartMs = -1L; | ||
| private long lastRebalanceEndMs = -1L; | ||
|
|
@@ -237,16 +237,6 @@ protected synchronized boolean ensureCoordinatorReady(final Timer timer) { | |
|
|
||
| if (!future.isDone()) { | ||
| // ran out of time | ||
| future.addListener(new RequestFutureListener<Void>() { | ||
| @Override | ||
| public void onSuccess(Void value) {} // do nothing | ||
|
|
||
| @Override | ||
| public void onFailure(RuntimeException e) { | ||
| findCoordinatorException = e; | ||
| } | ||
| }); | ||
|
|
||
| break; | ||
| } | ||
|
|
||
|
|
@@ -274,8 +264,20 @@ protected synchronized RequestFuture<Void> lookupCoordinator() { | |
| if (node == null) { | ||
| log.debug("No broker available to send FindCoordinator request"); | ||
| return RequestFuture.noBrokersAvailable(); | ||
| } else | ||
| } else{ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this probably breaks checkstyle |
||
| findCoordinatorFuture = sendFindCoordinatorRequest(node); | ||
| // remember the exception even after the future is cleared so that | ||
| // it can still be thrown by the ensureCoordinatorReady caller | ||
| findCoordinatorFuture.addListener(new RequestFutureListener<Void>() { | ||
| @Override | ||
| public void onSuccess(Void value) {} // do nothing | ||
|
|
||
| @Override | ||
| public void onFailure(RuntimeException e) { | ||
| findCoordinatorException = e; | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| return findCoordinatorFuture; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -769,6 +769,9 @@ public boolean refreshCommittedOffsetsIfNeeded(Timer timer) { | |
| this.subscriptions.seekUnvalidated(tp, position); | ||
|
|
||
| log.info("Setting offset for partition {} to the committed offset {}", tp, position); | ||
| } else { | ||
| log.info("Ignoring the returned {} since its partition {} is no longer assigned", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I'd suggest "Ignoring the fetched committed offset"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I originally did that, but then I realize |
||
| offsetAndMetadata, tp); | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
This seems a little self-contradictory... The next throws declaration says that if I'm not in the group, I can call
pollto rejoin and retry the commit. But this javadoc seems to suggest that if I get theCommitFailedException, I can definitely not retry the commit. Should it instead just simply say that I'm trying to commit partitions that I no longer own? I.e., that I am an active member of the group (because I have assigned partitions), but I don't own some of the partitions I'm trying to commit offsets on?Also, what happens if I do still own some other partitions? How can I know whether or not those got committed?
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.
If you try to commit and it fails because you no longer own some (or any) partitions because a rebalance just occurred, that's retriable. If you dropped out of the group and for that reason do not own those (or any) partitions, that's not retriable.
@guozhangwang I do agree that we could clarify the wording a bit: in particular, it seems to matter explicitly why the consumer no longer owns those partitions
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.
Yup, I've reworded the statement a bit.