KAFKA-14121: AlterPartitionReassignments API should allow callers to specify the option of preserving the replication factor #18983
Conversation
…specify the option of preserving the replication factor
| */ | ||
| public boolean allowReplicationFactorChange(boolean allow) { | ||
| this.allowReplicationFactorChange = allow; | ||
| return allowReplicationFactorChange; |
There was a problem hiding this comment.
On a quick cross-check with other *Options it appears that for such methods we return the *Options class itself. In other words, something along these lines
public AlterPartitionReassignmentsOptions allowReplicationFactorChange(boolean allow) {
this.allowReplicationFactorChange = allow;
return this;
}
| @@ -2406,6 +2414,31 @@ private void updatePartitionInfo( | |||
| newPartInfo.elr); | |||
| } | |||
|
|
|||
| private void validatePartitionReplicationFactorUnchanged( | |||
| PartitionRegistration part, | |||
There was a problem hiding this comment.
Nit: Could you align the first argument with the opening bracket as other methods in this file?
| @@ -432,6 +433,21 @@ topicPartition, new PartitionReassignmentState(asList(0, 1, 2), asList(0, 1, 2), | |||
| } | |||
| } | |||
|
|
|||
| @ClusterTest | |||
| public void testDisallowReplicationFactorChange() { | |||
There was a problem hiding this comment.
Could you also add a test (or build on this one) which showcases that increasing the replication factor also fails?
| setTopics(singletonList(new OngoingTopicReassignment(). | ||
| setName("foo").setPartitions(singletonList( | ||
| new OngoingPartitionReassignment().setPartitionIndex(0). | ||
| setRemovingReplicas(singletonList(3)). |
There was a problem hiding this comment.
Nit: Partition 0 started with replicas (0, 1, 2). For the sake of continuity, could you remove 0 and add 3 rather than the other way around?
| setTopics(singletonList(new OngoingTopicReassignment(). | ||
| setName("foo").setPartitions(singletonList( | ||
| new OngoingPartitionReassignment().setPartitionIndex(0). | ||
| setRemovingReplicas(singletonList(3)). |
There was a problem hiding this comment.
Nit: Same comment as in the previous test
| @@ -1922,6 +1922,135 @@ public void testReassignPartitions(short version) { | |||
| assertEquals(NONE_REASSIGNING, replication.listPartitionReassignments(null, Long.MAX_VALUE)); | |||
| } | |||
|
|
|||
| @ParameterizedTest | |||
| @ApiKeyVersionsSource(apiKey = ApiKeys.ALTER_PARTITION) | |||
| public void testAlterPartitionDisallowReplicationFactorChange(short version) { | |||
There was a problem hiding this comment.
Maybe there already are such tests, in which case please point them out to me, but if not, could you add the same tests but with setAllowReplicationFactorChange(true)?
KAFKA-14121
Add new option
allowReplicationFactorChangeto alterPartitionReassignments. Set this option to false prevents partition replication factor being changed during partition reassignment.See https://cwiki.apache.org/confluence/display/KAFKA/KIP-860%3A+Add+client-provided+option+to+guard+against+replication+factor+change+during+partition+reassignments for more detail
Committer Checklist (excluded from commit message)