KAFKA-5258: move all partition and replica state transition rules into their states #3071
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| Map(NewPartition -> Set(NonExistentPartition), | ||
| OnlinePartition -> Set(NewPartition, OnlinePartition, OfflinePartition), | ||
| OfflinePartition -> Set(NewPartition, OnlinePartition, OfflinePartition), | ||
| NonExistentPartition -> Set(OfflinePartition)) |
There was a problem hiding this comment.
Did you consider defining this in the PartitionState types? For example:
case object NewPartition extends PartitionState {
val state: Byte = 0
val validPreviousStates = Set(NonExistentPartition)
}If a new state is added, it will be clear (i.e. there will be a compiler error) that this method needs to be implemented.
There was a problem hiding this comment.
Great idea! I'll make the change.
…o their states Today the PartitionStateMachine and ReplicaStateMachine defines and asserts the valid state transitions inline for each state. It would be cleaner to move all partition and replica state transition rules into their states and simply do the assertion at the top of the handleStateChange.
onurkaraman
changed the title
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
| @@ -146,18 +146,17 @@ class PartitionStateMachine(controller: KafkaController) extends Logging { | |||
| val topicAndPartition = TopicAndPartition(topic, partition) | |||
| val currState = partitionState.getOrElseUpdate(topicAndPartition, NonExistentPartition) | |||
| try { | |||
| assertValidTransition(topicAndPartition, targetState) | |||
| targetState match { | |||
| case NewPartition => | |||
| // pre: partition did not exist before this | |||
There was a problem hiding this comment.
Hmm, should we remove all the pre comments? It seems like they duplicate what's encoded as the validPreviousStates. Also, if we want to add comments, we should add them in validPreviousStates as they are less likely to get out of date there.
There was a problem hiding this comment.
I'm fine with either option. Which do you prefer? By the way, sometimes there are post comments as well.
There was a problem hiding this comment.
If the pre comments just repeat what's in the code (which was the case for the ones I checked), I'd personally remove them. I saw the post comments as well. Since this PR is only about the valid transition states, I'd be OK with keeping things as they are for the post comments.
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
Today the PartitionStateMachine and ReplicaStateMachine defines and asserts the valid state transitions inline for each state. It would be cleaner to move all partition and replica state transition rules into a map and simply do the assertion at the top of the handleStateChange.