MINOR: Add upgrade notes for new consumer poll#5084
Conversation
|
cc @vvcephei |
hachikuji
force-pushed
the
minor-kip-266-followup
branch
from
617895e to
6865c50
Compare
|
I added a couple of comments regarding the documentation here: c470ff7#diff-e9c1ee46a19a8684d9d8d8a8c77f9005 Generally, it seems like the commit description is very thorough, but the documentation itself is sparse. Are we sure that some of the context in the commit description is not useful for users who have to:
|
There was a problem hiding this comment.
Ah, yeah, I forgot about these.
Add the new stricter-timeout version of `poll` proposed in KIP-266.
The pre-existing variant `poll(long timeout)` would block indefinitely for metadata
updates if they were needed, then it would issue a fetch and poll for `timeout` ms
for new records. The initial indefinite metadata block caused applications to become
stuck when the brokers became unavailable. The existence of the timeout parameter
made the indefinite block especially unintuitive.
This PR adds `poll(Duration timeout)` with the semantics:
1. iff a metadata update is needed:
1. send (asynchronous) metadata requests
2. poll for metadata responses (counts against timeout)
- if no response within timeout, **return an empty collection immediately**
2. if there is fetch data available, **return it immediately**
3. if there is no fetch request in flight, send fetch requests
4. poll for fetch responses (counts against timeout)
- if no response within timeout, **return an empty collection** (leaving async fetch request for the next poll)
- if we get a response, **return the response**
The old method, `poll(long timeout)` is deprecated, but we do not change its semantics, so it remains:
1. iff a metadata update is needed:
1. send (asynchronous) metadata requests
2. poll for metadata responses *indefinitely until we get it*
2. if there is fetch data available, **return it immediately**
3. if there is no fetch request in flight, send fetch requests
4. poll for fetch responses (counts against timeout)
- if no response within timeout, **return an empty collection** (leaving async fetch request for the next poll)
- if we get a response, **return the response**
One notable usage is prohibited by the new `poll`: previously, you could call `poll(0)` to block for metadata updates, for example to initialize the client, supposedly without fetching records. Note, though, that this behavior is not according to any contract, and there is no guarantee that `poll(0)` won't return records the first time it's called. Therefore, it has always been unsafe to ignore the response.
|
I agree with @ijuma that we could elaborate more in the JavaDoc in On On I'm not sure if more detail is necessary here. |
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
Could we piggy-back the comments I had in #4855 (comment) and #4855 (comment) in this PR as well?
|
Otherwise, LGTM. |
hachikuji
force-pushed
the
minor-kip-266-followup
branch
from
91ae732 to
616f6c0
Compare
|
@guozhangwang Thanks, I modified the field names of |
|
@guozhangwang Hold off on merging this. I will have a few additional changes after I merge #5014. |
There was a problem hiding this comment.
Do you need this currentTime because ensureCoordinatorReady, ensureFreshMetadata and ensureActiveGroup are somewhat long-running?
There was a problem hiding this comment.
Yes, right. I think the approach in #5087 is less irritating.
hachikuji
force-pushed
the
minor-kip-266-followup
branch
from
616f6c0 to
d76f802
Compare
vvcephei
left a comment
vvcephei
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for this patch.
|
LGTM! thanks. |
| * of the offset commit | ||
| */ | ||
| @Override | ||
| public void commitSync(Duration timeout) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Ack, will do. I've had the editor on that wiki open since this morning, but keep getting distracted.
This patch contains a few follow-up improvements/cleanup for KIP-266: - Add upgrade notes - Add missing `commitSync(Duration)` API - Improve timeout messages and fix some naming inconsistencies - Various small cleanups Reviewers: John Roesler <john@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
5 participants
Added upgrade notes for the new poll() API. I also added a few small cleanups from #4855.
Committer Checklist (excluded from commit message)