KAFKA-19400: Update AddRaftVoterRequest RPC to version 1 #19982
Conversation
|
|
||
| // This method sets up the context so a test can send an AddVoter request after | ||
| // exiting this method | ||
| private void prepareToSendAddVoter( |
There was a problem hiding this comment.
this name is a bit deceiving, this is all prep we do on the local (leader) node in order for it to be able to respond favorably to an addvoter request, could we perhaps rename to "prepareLeaderToReceiveAddVoter"?
I'm also not sure how I feel about having a helper method for this, I do see how this is quite a bit of code duplication but I wonder if it might be more clear to have this written out explicitly in the tests (you probably don't need to keep all the same assertions for all the tests)
There was a problem hiding this comment.
could we perhaps rename to "prepareLeaderToReceiveAddVoter"
Yeah, this is more accurate.
I wonder if it might be more clear to have this written out explicitly in the tests
I would prefer to have this code in a helper method, which applies to the other comment too. In my opinion, it's clearer to me when all of these tests do have the exact same assertions so long as they apply, because it means our new AckWhenCommitted field (or any other feature) is not unintentionally changing behavior elsewhere. For example, the metric values check prevented me from writing an incorrect implementation!
Another way to think about it is that in these AddVoter unit tests, we're testing the same state of KRaft essentially (i.e. how the local leader handles AddVoter RPC), but each test is changing one thing, which is the ackWhenCommitted value (true, false, or NOT_SUPPORTED). Because we want coverage, I think duplication is a natural side-effect.
Maybe I can just document the helpers/rename them as specific to these AddVoter tests?
| ReplicaKey follower, | ||
| ReplicaKey newVoter, | ||
| int epoch | ||
| ) throws Exception { | ||
| // The new voter is now a voter after writing the VotersRecord to the log | ||
| assertTrue(context.client.quorum().isVoter(newVoter)); | ||
| checkLeaderMetricValues(3, 0, 1, context); |
There was a problem hiding this comment.
this doesn't seem very extensible - maybe the above checks belong outside of this method
| "KIP_853_PROTOCOL", | ||
| "KIP_996_PROTOCOL" | ||
| }) | ||
| void testAddVoterCompatibility(RaftProtocol protocol) throws Exception { |
There was a problem hiding this comment.
testAddVoterAckWhenCommittedUnsupported?
There was a problem hiding this comment.
I also need to add KIP_1166 here too.
Add the
ackWhenCommittedboolean field to theAddRaftVoterRequestRPC, and bump the RPC's version to 1.ackWhenCommittedistrue, and in this case the leader will return a response after committing theVotersRecordgenerated by the RPC.ackWhenCommitted == false, the leader will return a response after writing theVotersRecordto its own local log.KafkaRaftClientReconfigTest