KAFKA-12158: Better return type of RaftClient.scheduleAppend#10909
KAFKA-12158: Better return type of RaftClient.scheduleAppend#10909hachikuji merged 7 commits intoapache:trunkfrom
Conversation
005201a to
4bab640
Compare
4bab640 to
b3e360d
Compare
|
cc @jsancio |
b3e360d to
21b8f09
Compare
bc12045 to
e56546e
Compare
39d479c to
e1131c9
Compare
jsancio
left a comment
There was a problem hiding this comment.
Thanks for the changes @dengziming
hachikuji
left a comment
There was a problem hiding this comment.
Thanks for the updates. Left a few suggestions.
raft/src/main/java/org/apache/kafka/raft/ReplicatedCounter.java
Outdated
Show resolved
Hide resolved
raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java
Outdated
Show resolved
Hide resolved
2ace2d6 to
b2c1cbb
Compare
jsancio
left a comment
There was a problem hiding this comment.
Thanks for the chances, just a few minor suggestions.
There was a problem hiding this comment.
I think it would be nice if we could consolidate NotLeaderException and FencedEpochException. In the context of scheduleAppend, they mean the same thing to the caller. They both say that the raft state has transitioned and the operation is no longer possible. I'm inclined to keep NotLeaderException and document that it covers both cases. What do you think?
There was a problem hiding this comment.
Here I referred to the practices of KafkaController in which we have NOT_CONTROLLER and STALE_CONTROLLER_EPOCH for RPC errors, but here I also think it's better to consolidate NotLeaderException and FencedEpochException now.
jsancio
left a comment
There was a problem hiding this comment.
One observation, otherwise LGTM.
There was a problem hiding this comment.
@dengziming @hachikuji What do you both think about making this consistent with RaftClient::resign? RaftClient::resign throws IllegalArgumentException when the passed epoch is greater than the current epoch. With this change KafkaRaftClient::schedule*Append throws NotLeaderException when the passed epoch is greater than the current epoch.
Maybe something like:
if (epoch > this.epoch) {
throw new IllegalArgumentExcpetion("...");
} else if (epoch < this.epoch) {
throw new NotLeaderException("...");
}
There was a problem hiding this comment.
I think this is a good suggestion, only an epoch smaller than the current leader epoch should be treated as NotLeaderException.
2aa8e31 to
fdf8768
Compare
hachikuji
left a comment
There was a problem hiding this comment.
LGTM. Pushed one trivial change in ReplicatedCounter.
This patch improves the return type for `scheduleAppend` and `scheduleAtomicAppend`. Previously we were using a `Long` value and using both `null` and `Long.MaxValue` to distinguish between different error cases. In this PR, we change the return type to `long` and only return a value if the append was accepted. For the error cases, we instead throw an exception. For this purpose, the patch introduces a couple new exception types: `BufferAllocationException` and `NotLeaderException`. Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>, Jason Gustafson <jason@confluent.io>
…10909) This patch improves the return type for `scheduleAppend` and `scheduleAtomicAppend`. Previously we were using a `Long` value and using both `null` and `Long.MaxValue` to distinguish between different error cases. In this PR, we change the return type to `long` and only return a value if the append was accepted. For the error cases, we instead throw an exception. For this purpose, the patch introduces a couple new exception types: `BufferAllocationException` and `NotLeaderException`. Reviewers: José Armando García Sancio <jsancio@users.noreply.github.com>, Jason Gustafson <jason@confluent.io>
More detailed description of your change
It's not very convenient to return Long for
RaftClient.scheduleAppend, in this PR we add aRaftAppendResultclass to replace it.I also found that we lack logic on append failure, for example, the
ControllerWriteEventwon't check the result ofraftClient.scheduleAppend, this should be fixed in a future PR.Summary of testing strategy (including rationale)
QA