KAFKA-16907: Fix RaftUtil's type complexity #16831
Conversation
|
Will fix conflicts after 3.9 RC |
…Complexity issue caused by RaftUtils
frankvicky
force-pushed
the
KAFKA-16907
branch
from
d4b745c to
b31643b
Compare
|
Hi @jsancio, |
|
Hi @chia7712 |
| } | ||
| } | ||
|
|
||
| public static AddRaftVoterRequestData addVoterRequest( |
There was a problem hiding this comment.
VoteRequest, VoteResponse should be part of the VoteRPCs, but not addVoter, removeVoter, updateVoter. Perhaps we want a DynamicReconfigRpc class for those
| @@ -288,7 +290,7 @@ private ApiMessage buildTestRequest(ApiKeys key) { | |||
| return VoteRequest.singletonRequest(topicPartition, clusterId, leaderEpoch, leaderId, lastEpoch, 329); | |||
|
|
|||
| case FETCH: | |||
| FetchRequestData request = RaftUtil.singletonFetchRequest(topicPartition, topicId, fetchPartition -> | |||
| FetchRequestData request = FetchRpc.singletonFetchRequest(topicPartition, topicId, fetchPartition -> | |||
There was a problem hiding this comment.
I wouldn't be upset having both Fetch and FetchSnapshot in the same class. But I guess same argument could be made for Begin and End Quorum (and I see how naming might get more difficult)
| data.topics().get(0).topicName().equals(topicPartition.topic()) && | ||
| data.topics().get(0).partitions().size() == 1 && | ||
| data.topics().get(0).partitions().get(0).partitionIndex() == topicPartition.partition(); | ||
| } | ||
|
|
||
| public static boolean hasValidTopicPartition(BeginQuorumEpochResponseData data, TopicPartition topicPartition) { | ||
| return data.topics().size() == 1 && | ||
| data.topics().get(0).topicName().equals(topicPartition.topic()) && | ||
| data.topics().get(0).partitions().size() == 1 && | ||
| data.topics().get(0).partitions().get(0).partitionIndex() == topicPartition.partition(); |
There was a problem hiding this comment.
It seems like an issue of idea format config.
I have adjusted it.
| TopicPartition topicPartition | ||
| ) { | ||
| return new DescribeQuorumRequestData() | ||
| .setTopics( |
There was a problem hiding this comment.
looks like these are all 8 vs 4 spaces
frankvicky
force-pushed
the
KAFKA-16907
branch
from
7b2740b to
25bff77
Compare
| .setLeaderEpoch(leaderEpoch) | ||
| .setHighWatermark(highWatermark) | ||
| .setCurrentVoters(toReplicaStates(apiVersion, leaderId, voters, currentTimeMs)) | ||
| .setObservers(toReplicaStates(apiVersion, leaderId, observers, currentTimeMs)))))); |
There was a problem hiding this comment.
style nit: I know this was carried over from the old code, but maybe better to have each closing parenthesis on its own new line
(there are a few other spots in the PR which could use this too)
There was a problem hiding this comment.
I will take a look 😺
frankvicky
force-pushed
the
KAFKA-16907
branch
from
74df38e to
25c6477
Compare
JIRA: KAFKA-16907
This PR aims to fix the
ClassDataAbstractionCouplingandClassFanOutComplexityissues inRaftUtils.RaftUtilswill be split into several classes, each based on a specific RPC in the Raft consensus protocol.Committer Checklist (excluded from commit message)