KAFKA-13002: listOffsets must downgrade immediately for non MAX_TIMESTAMP specs#10936
KAFKA-13002: listOffsets must downgrade immediately for non MAX_TIMESTAMP specs#10936dajac merged 22 commits intoapache:trunkfrom
Conversation
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
@thomaskwscott Thanks for the prompt fix. I left a few minor comments.
dajac
left a comment
dajac
left a comment
There was a problem hiding this comment.
@thomaskwscott Thanks for the update. I left few comments.
|
|
||
| env.kafkaClient().setNodeApiVersions(NodeApiVersions.create()); | ||
| env.kafkaClient().setNodeApiVersions(NodeApiVersions.create( | ||
| ApiKeys.LIST_OFFSETS.id, (short) 0, (short) 6)); |
There was a problem hiding this comment.
I think that we should use 0 for the maximum version in this case as 0 does not support querying by timestamp at all.
| env.kafkaClient().setNodeApiVersions(NodeApiVersions.create( | ||
| ApiKeys.LIST_OFFSETS.id, (short) 0, (short) 6)); |
There was a problem hiding this comment.
It is a bit annoying that we have to keep calling prepareUnsupportedVersionResponse below even when we provide the versions. In the end, setting the versions in this case does not change much because prepareUnsupportedVersionResponse will make the request fail anyway. This is inherently due to the implementation of the MockClient which requires queued responses in order to process queued requests. It is fine to keep it as it in this patch but we should really rework the MockClient.
|
I was able to reproduce the issue running the stream wordcount demo on current trunk with 2.8.0 brokers. I can confirm that the latest build of this branch resolves it. |
|
@thomaskwscott Some tests failed. It might be related to the changes in the MockClient. Could you verify? |
dajac
changed the title
…TAMP specs (apache#10936) This patch fixes a regression introduced apache#10760. The downgrade logic was not downgrading the version when only non MAX_TIMESTAMP specs were used. Reviewers: David Jacot <djacot@confluent.io>
2 participants
KAFKA-12541 introduced a regression for listOffsets requests for non maxtimestamp specs. when communicating with old brokers. This PR addresss this case.
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Tested with new unit test for regression case.
Committer Checklist (excluded from commit message)