KAFKA-9630; Replace OffsetsForLeaderEpoch request/response with automated protocol#9547
KAFKA-9630; Replace OffsetsForLeaderEpoch request/response with automated protocol#9547dajac merged 9 commits intoapache:trunkfrom
Conversation
dajac
changed the title
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Looks good, just a couple comments.
There was a problem hiding this comment.
Wonder if it might be simpler to initialize partitionsToRetry from the request key set.
There was a problem hiding this comment.
The downside of relying on partitionsToRetry is that we can't differentiate the partitions to be retried due to an error from the missing partitions in the response. It may not be that important but we can't preserve the above log otherwise.
logger().warn("Missing partition {} from response, retrying.", topicPartition);
I suppose that this log entry is there for a reason so I went with preserving it. What do you think?
There was a problem hiding this comment.
No strong opinion. It's an unlikely case anyway, so I'm not sure it calls for special treatment.
There was a problem hiding this comment.
Yeah, you're right. Let me remove it.
There was a problem hiding this comment.
We have similar logic in OffsetsForLeaderEpochClient.prepareRequest. Wonder if we should push it to the Builder?
There was a problem hiding this comment.
That's a good question. Actually, I was hoping to push this one out of the builder while removing PartitionData in profit of using the internal data structure of the auto-generated protocol. I'd like to do this in a follow-up PR.
There was a problem hiding this comment.
Sounds fine. Do you have a jira?
There was a problem hiding this comment.
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
I like this great patch. +1
some trivial comments are left. please take a look :)
There was a problem hiding this comment.
not sure why this kind of method still exist in each request. It is not used anymore.
There was a problem hiding this comment.
Good question. As we have kept them in other requests/responses, I will keep it for now. We should consider removing all of them.
There was a problem hiding this comment.
I think these are still used in RequestResponseTest. There's probably a lot of cruft like this that we can start cleaning up though.
There was a problem hiding this comment.
Is it worth using data rather than responses() to avoid extra conversion?
There was a problem hiding this comment.
I hope we can get rid of those conversion in the future :)
There was a problem hiding this comment.
I will address this in a follow-up PR.
OffsetForLeaderEpoch and Produce are not yet generated RPCs, but will be once apache#9401 and apache#9547 are merged.
|
@chia7712 @hachikuji Thanks for your comments. I have addressed them. Could you have another quick look? |
|
@dajac The updates LGTM. |
|
@dajac +1 to nice updating. sorry that my commit causes conflicting files to you. |
…tead of EpochEndOffset.
|
Failed test seems unrelated to this PR. Merging to trunk. |
This patch follows up #9547. It refactors KafkaApis, ReplicaManager and Partition to use `OffsetForLeaderEpochResponseData.EpochEndOffset` instead of `EpochEndOffset`. In the mean time, it removes `OffsetsForLeaderEpochRequest#epochsByTopicPartition` and `OffsetsForLeaderEpochResponse#responses` and replaces their usages to use the automated protocol directly. Finally, it removes old constructors in `OffsetsForLeaderEpochResponse`. The patch relies on existing tests. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jason Gustafson <jason@confluent.io>
…automated protocol (#9689) This patch follows up #9547. It refactors AbstractFetcherThread and its descendants to use `OffsetForLeaderEpochRequestData.OffsetForLeaderPartition` instead of `OffsetsForLeaderEpochRequest.PartitionData`. The patch relies on existing tests. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Jason Gustafson <jason@confluent.io>
3 participants
This PR migrates the
OffsetsForLeaderEpochrequest/response to the automated protocol. It also refactors theOffsetsForLeaderEpochClientto use directly the internal structs generated by the automated protocol. It relies on the existing tests.It seems that we could also refactor the broker (api layer, fetcher) to directly use the internal structs of the generated protocol but, as it is more involved, I propose to address it in a separate PR.
Committer Checklist (excluded from commit message)