You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "andymg3 (via GitHub)" <gi...@apache.org> on 2023/05/31 21:01:57 UTC
[GitHub] [kafka] andymg3 opened a new pull request, #13788: KAFKA-14791: Create a builder for PartitionRegistration
andymg3 opened a new pull request, #13788:
URL: https://github.com/apache/kafka/pull/13788
### JIRA
https://issues.apache.org/jira/browse/KAFKA-14791
### Description
This creates a builder for `PartitionRegistration`. The motivation for the builder is that the constructor of `PartitionRegistration` has four arguments all of type `int[]` which makes it easy to make a mistake when using it. Right now the constructor has the signature:
```
public PartitionRegistration(int[] replicas, int[] isr, int[] removingReplicas,
int[] addingReplicas, int leader, LeaderRecoveryState leaderRecoveryState,
int leaderEpoch, int partitionEpoch) {
```
It's easy to mix up any of `replicas`, `isr`, `removingReplicas` and `addingReplicas`.
With this change the constructor is made `protected` so only code in the same package can use it, which is helpful for unit testing. All other code must use the builder. The code will look like:
```
partitionRegistration = new PartitionRegistration.Builder().
setReplicas(Replicas.toArray(assignment.brokerIds())).
setIsr(Replicas.toArray(isr)).
setRemovingReplicas(Replicas.NONE).
setAddingReplicas(Replicas.NONE).
setLeader(isr.get(0)).
setLeaderRecoveryState(LeaderRecoveryState.RECOVERED).
setLeaderEpoch(0).
setPartitionEpoch(0).
build();
```
This makes it less likely to make a mistake when setting any of `replicas`, `isr`, `removingReplicas` and `addingReplicas`.
### Testing
This updates all the unit tests to use the new builder. This also adds unit tests to `PartitionRegistrationTest` testing the builder's validation and object constructions.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1212435786
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -57,7 +143,7 @@ public PartitionRegistration(PartitionRecord record) {
record.partitionEpoch());
}
- public PartitionRegistration(int[] replicas, int[] isr, int[] removingReplicas,
+ protected PartitionRegistration(int[] replicas, int[] isr, int[] removingReplicas,
Review Comment:
Let's make this private. We should discourage inheritance of this class.
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -33,6 +33,92 @@
public class PartitionRegistration {
+
+ /**
+ * A builder class which creates a PartitionRegistration.
+ */
+ static public class Builder {
+ private int[] replicas;
+ private int[] isr;
+ private int[] removingReplicas;
+ private int[] addingReplicas;
+ private Integer leader;
+ private LeaderRecoveryState leaderRecoveryState;
+ private Integer leaderEpoch;
+ private Integer partitionEpoch;
+
+ public Builder setReplicas(int[] replicas) {
+ this.replicas = replicas;
+ return this;
+ }
+
+ public Builder setIsr(int[] isr) {
+ this.isr = isr;
+ return this;
+ }
+
+ public Builder setRemovingReplicas(int[] removingReplicas) {
+ this.removingReplicas = removingReplicas;
+ return this;
+ }
+
+ public Builder setAddingReplicas(int[] addingReplicas) {
+ this.addingReplicas = addingReplicas;
+ return this;
+ }
+
+ public Builder setLeader(Integer leader) {
+ this.leader = leader;
+ return this;
+ }
+
+ public Builder setLeaderRecoveryState(LeaderRecoveryState leaderRecoveryState) {
+ this.leaderRecoveryState = leaderRecoveryState;
+ return this;
+ }
+
+ public Builder setLeaderEpoch(Integer leaderEpoch) {
+ this.leaderEpoch = leaderEpoch;
+ return this;
+ }
+
+ public Builder setPartitionEpoch(Integer partitionEpoch) {
+ this.partitionEpoch = partitionEpoch;
+ return this;
+ }
+
+ public PartitionRegistration build() throws Exception {
Review Comment:
You should be able to remove the `throws Exception` since `IllegalStateException` is a `RuntimeException`.
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -77,39 +77,75 @@ public void testChangeRecordIsNoOp() {
);
}
- private final static PartitionRegistration FOO = new PartitionRegistration(
- new int[] {2, 1, 3}, new int[] {2, 1, 3}, Replicas.NONE, Replicas.NONE,
- 1, LeaderRecoveryState.RECOVERED, 100, 200);
+ private static final PartitionRegistration FOO;
+
+ static {
+ try {
+ FOO = new PartitionRegistration.Builder().
+ setReplicas(new int[] {2, 1, 3}).
+ setIsr(new int[] {2, 1, 3}).
+ setRemovingReplicas(Replicas.NONE).
+ setAddingReplicas(Replicas.NONE).
Review Comment:
We can remove this if we make the default value for these two fields `Replicas.NONE`.
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -77,39 +77,75 @@ public void testChangeRecordIsNoOp() {
);
}
- private final static PartitionRegistration FOO = new PartitionRegistration(
- new int[] {2, 1, 3}, new int[] {2, 1, 3}, Replicas.NONE, Replicas.NONE,
- 1, LeaderRecoveryState.RECOVERED, 100, 200);
+ private static final PartitionRegistration FOO;
+
+ static {
+ try {
+ FOO = new PartitionRegistration.Builder().
+ setReplicas(new int[] {2, 1, 3}).
+ setIsr(new int[] {2, 1, 3}).
+ setRemovingReplicas(Replicas.NONE).
+ setAddingReplicas(Replicas.NONE).
+ setLeader(1).
+ setLeaderRecoveryState(LeaderRecoveryState.RECOVERED).
+ setLeaderEpoch(100).
+ setPartitionEpoch(200).
+ build();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
Isn't `IllegalStateExection` a runtime exception?
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -33,6 +33,92 @@
public class PartitionRegistration {
+
+ /**
+ * A builder class which creates a PartitionRegistration.
+ */
+ static public class Builder {
+ private int[] replicas;
+ private int[] isr;
+ private int[] removingReplicas;
+ private int[] addingReplicas;
+ private Integer leader;
+ private LeaderRecoveryState leaderRecoveryState;
+ private Integer leaderEpoch;
+ private Integer partitionEpoch;
+
+ public Builder setReplicas(int[] replicas) {
+ this.replicas = replicas;
+ return this;
+ }
+
+ public Builder setIsr(int[] isr) {
+ this.isr = isr;
+ return this;
+ }
+
+ public Builder setRemovingReplicas(int[] removingReplicas) {
+ this.removingReplicas = removingReplicas;
+ return this;
+ }
+
+ public Builder setAddingReplicas(int[] addingReplicas) {
+ this.addingReplicas = addingReplicas;
+ return this;
+ }
+
+ public Builder setLeader(Integer leader) {
+ this.leader = leader;
+ return this;
+ }
+
+ public Builder setLeaderRecoveryState(LeaderRecoveryState leaderRecoveryState) {
+ this.leaderRecoveryState = leaderRecoveryState;
+ return this;
+ }
+
+ public Builder setLeaderEpoch(Integer leaderEpoch) {
+ this.leaderEpoch = leaderEpoch;
+ return this;
+ }
+
+ public Builder setPartitionEpoch(Integer partitionEpoch) {
+ this.partitionEpoch = partitionEpoch;
+ return this;
+ }
+
+ public PartitionRegistration build() throws Exception {
+ if (replicas == null) {
+ throw new IllegalStateException("You must set replicas.");
+ } else if (isr == null) {
+ throw new IllegalStateException("You must set isr.");
+ } else if (removingReplicas == null) {
+ throw new IllegalStateException("You must set removing replicas.");
+ } else if (addingReplicas == null) {
+ throw new IllegalStateException("You must set adding replicas.");
Review Comment:
Can we default this to be the empty array: `int[0]`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] andymg3 commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "andymg3 (via GitHub)" <gi...@apache.org>.
andymg3 commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1218539167
##########
core/src/test/scala/unit/kafka/zk/migration/ZkMigrationClientTest.scala:
##########
@@ -371,17 +371,17 @@ class ZkMigrationClientTest extends ZkMigrationTestHarness {
val topicId = Uuid.randomUuid()
val partitions = Map(
- 0 -> new PartitionRegistration(Array(0, 1, 2), Array(0, 1, 2), Array(), Array(), 0, LeaderRecoveryState.RECOVERED, 0, -1),
- 1 -> new PartitionRegistration(Array(1, 2, 3), Array(1, 2, 3), Array(), Array(), 1, LeaderRecoveryState.RECOVERED, 0, -1)
+ 0 -> new PartitionRegistration.Builder().setReplicas(Array(0, 1, 2)).setIsr(Array(0, 1, 2)).setLeader(0).setLeaderRecoveryState(LeaderRecoveryState.RECOVERED).setLeaderEpoch(0).setPartitionEpoch(1).build(),
Review Comment:
It apparently doesnt affect the test. But I agree it should be -1 for consistency. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] andymg3 commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "andymg3 (via GitHub)" <gi...@apache.org>.
andymg3 commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1212980561
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -77,39 +77,75 @@ public void testChangeRecordIsNoOp() {
);
}
- private final static PartitionRegistration FOO = new PartitionRegistration(
- new int[] {2, 1, 3}, new int[] {2, 1, 3}, Replicas.NONE, Replicas.NONE,
- 1, LeaderRecoveryState.RECOVERED, 100, 200);
+ private static final PartitionRegistration FOO;
+
+ static {
+ try {
+ FOO = new PartitionRegistration.Builder().
+ setReplicas(new int[] {2, 1, 3}).
+ setIsr(new int[] {2, 1, 3}).
+ setRemovingReplicas(Replicas.NONE).
+ setAddingReplicas(Replicas.NONE).
Review Comment:
Done here and all the other places.
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -33,6 +33,92 @@
public class PartitionRegistration {
+
+ /**
+ * A builder class which creates a PartitionRegistration.
+ */
+ static public class Builder {
+ private int[] replicas;
+ private int[] isr;
+ private int[] removingReplicas;
+ private int[] addingReplicas;
+ private Integer leader;
+ private LeaderRecoveryState leaderRecoveryState;
+ private Integer leaderEpoch;
+ private Integer partitionEpoch;
+
+ public Builder setReplicas(int[] replicas) {
+ this.replicas = replicas;
+ return this;
+ }
+
+ public Builder setIsr(int[] isr) {
+ this.isr = isr;
+ return this;
+ }
+
+ public Builder setRemovingReplicas(int[] removingReplicas) {
+ this.removingReplicas = removingReplicas;
+ return this;
+ }
+
+ public Builder setAddingReplicas(int[] addingReplicas) {
+ this.addingReplicas = addingReplicas;
+ return this;
+ }
+
+ public Builder setLeader(Integer leader) {
+ this.leader = leader;
+ return this;
+ }
+
+ public Builder setLeaderRecoveryState(LeaderRecoveryState leaderRecoveryState) {
+ this.leaderRecoveryState = leaderRecoveryState;
+ return this;
+ }
+
+ public Builder setLeaderEpoch(Integer leaderEpoch) {
+ this.leaderEpoch = leaderEpoch;
+ return this;
+ }
+
+ public Builder setPartitionEpoch(Integer partitionEpoch) {
+ this.partitionEpoch = partitionEpoch;
+ return this;
+ }
+
+ public PartitionRegistration build() throws Exception {
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] andymg3 commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "andymg3 (via GitHub)" <gi...@apache.org>.
andymg3 commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1212978727
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -33,6 +33,92 @@
public class PartitionRegistration {
+
+ /**
+ * A builder class which creates a PartitionRegistration.
+ */
+ static public class Builder {
+ private int[] replicas;
+ private int[] isr;
+ private int[] removingReplicas;
+ private int[] addingReplicas;
+ private Integer leader;
+ private LeaderRecoveryState leaderRecoveryState;
+ private Integer leaderEpoch;
+ private Integer partitionEpoch;
+
+ public Builder setReplicas(int[] replicas) {
+ this.replicas = replicas;
+ return this;
+ }
+
+ public Builder setIsr(int[] isr) {
+ this.isr = isr;
+ return this;
+ }
+
+ public Builder setRemovingReplicas(int[] removingReplicas) {
+ this.removingReplicas = removingReplicas;
+ return this;
+ }
+
+ public Builder setAddingReplicas(int[] addingReplicas) {
+ this.addingReplicas = addingReplicas;
+ return this;
+ }
+
+ public Builder setLeader(Integer leader) {
+ this.leader = leader;
+ return this;
+ }
+
+ public Builder setLeaderRecoveryState(LeaderRecoveryState leaderRecoveryState) {
+ this.leaderRecoveryState = leaderRecoveryState;
+ return this;
+ }
+
+ public Builder setLeaderEpoch(Integer leaderEpoch) {
+ this.leaderEpoch = leaderEpoch;
+ return this;
+ }
+
+ public Builder setPartitionEpoch(Integer partitionEpoch) {
+ this.partitionEpoch = partitionEpoch;
+ return this;
+ }
+
+ public PartitionRegistration build() throws Exception {
+ if (replicas == null) {
+ throw new IllegalStateException("You must set replicas.");
+ } else if (isr == null) {
+ throw new IllegalStateException("You must set isr.");
+ } else if (removingReplicas == null) {
+ throw new IllegalStateException("You must set removing replicas.");
+ } else if (addingReplicas == null) {
+ throw new IllegalStateException("You must set adding replicas.");
Review Comment:
Done. Used `Replicas.NONE`.
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -57,7 +143,7 @@ public PartitionRegistration(PartitionRecord record) {
record.partitionEpoch());
}
- public PartitionRegistration(int[] replicas, int[] isr, int[] removingReplicas,
+ protected PartitionRegistration(int[] replicas, int[] isr, int[] removingReplicas,
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on PR #13788:
URL: https://github.com/apache/kafka/pull/13788#issuecomment-1578924954
Merging. Unrelated rest failures:
```
testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1213346871
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -33,6 +33,92 @@
public class PartitionRegistration {
+
+ /**
+ * A builder class which creates a PartitionRegistration.
+ */
+ static public class Builder {
+ private int[] replicas;
+ private int[] isr;
+ private int[] removingReplicas = Replicas.NONE;
+ private int[] addingReplicas = Replicas.NONE;
+ private Integer leader;
+ private LeaderRecoveryState leaderRecoveryState;
+ private Integer leaderEpoch;
+ private Integer partitionEpoch;
+
+ public Builder setReplicas(int[] replicas) {
+ this.replicas = replicas;
+ return this;
+ }
+
+ public Builder setIsr(int[] isr) {
+ this.isr = isr;
+ return this;
+ }
+
+ public Builder setRemovingReplicas(int[] removingReplicas) {
+ this.removingReplicas = removingReplicas;
+ return this;
+ }
+
+ public Builder setAddingReplicas(int[] addingReplicas) {
+ this.addingReplicas = addingReplicas;
+ return this;
+ }
+
+ public Builder setLeader(Integer leader) {
+ this.leader = leader;
+ return this;
+ }
+
+ public Builder setLeaderRecoveryState(LeaderRecoveryState leaderRecoveryState) {
+ this.leaderRecoveryState = leaderRecoveryState;
+ return this;
+ }
+
+ public Builder setLeaderEpoch(Integer leaderEpoch) {
+ this.leaderEpoch = leaderEpoch;
+ return this;
+ }
+
+ public Builder setPartitionEpoch(Integer partitionEpoch) {
+ this.partitionEpoch = partitionEpoch;
+ return this;
+ }
+
+ public PartitionRegistration build() {
+ if (replicas == null) {
+ throw new IllegalStateException("You must set replicas.");
+ } else if (isr == null) {
+ throw new IllegalStateException("You must set isr.");
+ } else if (removingReplicas == null) {
+ throw new IllegalStateException("You must set removing replicas.");
+ } else if (addingReplicas == null) {
+ throw new IllegalStateException("You must set adding replicas.");
+ } else if (leader == null) {
+ throw new IllegalStateException("You must set leader.");
+ } else if (leaderRecoveryState == null) {
+ throw new IllegalStateException("You must set leader recovery state.");
Review Comment:
We discussed this PR offline. The change is good as is and there is not much benefits of adding addition default values to the builder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio merged pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio merged PR #13788:
URL: https://github.com/apache/kafka/pull/13788
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] andymg3 commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "andymg3 (via GitHub)" <gi...@apache.org>.
andymg3 commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1213276160
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -33,6 +33,92 @@
public class PartitionRegistration {
+
+ /**
+ * A builder class which creates a PartitionRegistration.
+ */
+ static public class Builder {
+ private int[] replicas;
+ private int[] isr;
+ private int[] removingReplicas = Replicas.NONE;
+ private int[] addingReplicas = Replicas.NONE;
+ private Integer leader;
+ private LeaderRecoveryState leaderRecoveryState;
+ private Integer leaderEpoch;
+ private Integer partitionEpoch;
+
+ public Builder setReplicas(int[] replicas) {
+ this.replicas = replicas;
+ return this;
+ }
+
+ public Builder setIsr(int[] isr) {
+ this.isr = isr;
+ return this;
+ }
+
+ public Builder setRemovingReplicas(int[] removingReplicas) {
+ this.removingReplicas = removingReplicas;
+ return this;
+ }
+
+ public Builder setAddingReplicas(int[] addingReplicas) {
+ this.addingReplicas = addingReplicas;
+ return this;
+ }
+
+ public Builder setLeader(Integer leader) {
+ this.leader = leader;
+ return this;
+ }
+
+ public Builder setLeaderRecoveryState(LeaderRecoveryState leaderRecoveryState) {
+ this.leaderRecoveryState = leaderRecoveryState;
+ return this;
+ }
+
+ public Builder setLeaderEpoch(Integer leaderEpoch) {
+ this.leaderEpoch = leaderEpoch;
+ return this;
+ }
+
+ public Builder setPartitionEpoch(Integer partitionEpoch) {
+ this.partitionEpoch = partitionEpoch;
+ return this;
+ }
+
+ public PartitionRegistration build() {
+ if (replicas == null) {
+ throw new IllegalStateException("You must set replicas.");
+ } else if (isr == null) {
+ throw new IllegalStateException("You must set isr.");
+ } else if (removingReplicas == null) {
+ throw new IllegalStateException("You must set removing replicas.");
+ } else if (addingReplicas == null) {
+ throw new IllegalStateException("You must set adding replicas.");
+ } else if (leader == null) {
+ throw new IllegalStateException("You must set leader.");
+ } else if (leaderRecoveryState == null) {
+ throw new IllegalStateException("You must set leader recovery state.");
Review Comment:
Should we go a step further and not require `leader`, `isr`, `leaderEpoch` and `partitionEpoch`?
We could default to using the first replica in `replicas` for the leader. We could default to all replicas being in the ISR and we could default `leaderEpoch` and `partitionEpoch` to 0? It looks like we really are just calling `build` during topic creation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1218527800
##########
core/src/test/scala/unit/kafka/zk/migration/ZkMigrationClientTest.scala:
##########
@@ -371,17 +371,17 @@ class ZkMigrationClientTest extends ZkMigrationTestHarness {
val topicId = Uuid.randomUuid()
val partitions = Map(
- 0 -> new PartitionRegistration(Array(0, 1, 2), Array(0, 1, 2), Array(), Array(), 0, LeaderRecoveryState.RECOVERED, 0, -1),
- 1 -> new PartitionRegistration(Array(1, 2, 3), Array(1, 2, 3), Array(), Array(), 1, LeaderRecoveryState.RECOVERED, 0, -1)
+ 0 -> new PartitionRegistration.Builder().setReplicas(Array(0, 1, 2)).setIsr(Array(0, 1, 2)).setLeader(0).setLeaderRecoveryState(LeaderRecoveryState.RECOVERED).setLeaderEpoch(0).setPartitionEpoch(1).build(),
Review Comment:
This should `setPartitionEpoch(-1)`, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] andymg3 commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "andymg3 (via GitHub)" <gi...@apache.org>.
andymg3 commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1212980304
##########
metadata/src/test/java/org/apache/kafka/controller/PartitionChangeBuilderTest.java:
##########
@@ -77,39 +77,75 @@ public void testChangeRecordIsNoOp() {
);
}
- private final static PartitionRegistration FOO = new PartitionRegistration(
- new int[] {2, 1, 3}, new int[] {2, 1, 3}, Replicas.NONE, Replicas.NONE,
- 1, LeaderRecoveryState.RECOVERED, 100, 200);
+ private static final PartitionRegistration FOO;
+
+ static {
+ try {
+ FOO = new PartitionRegistration.Builder().
+ setReplicas(new int[] {2, 1, 3}).
+ setIsr(new int[] {2, 1, 3}).
+ setRemovingReplicas(Replicas.NONE).
+ setAddingReplicas(Replicas.NONE).
+ setLeader(1).
+ setLeaderRecoveryState(LeaderRecoveryState.RECOVERED).
+ setLeaderEpoch(100).
+ setPartitionEpoch(200).
+ build();
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
Yeah. I've changed the signature to not have `throws Exception` so I've removed the catches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jsancio commented on a diff in pull request #13788: KAFKA-14791: Create a builder for PartitionRegistration
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #13788:
URL: https://github.com/apache/kafka/pull/13788#discussion_r1213240584
##########
metadata/src/main/java/org/apache/kafka/metadata/PartitionRegistration.java:
##########
@@ -33,6 +33,92 @@
public class PartitionRegistration {
+
+ /**
+ * A builder class which creates a PartitionRegistration.
+ */
+ static public class Builder {
+ private int[] replicas;
+ private int[] isr;
+ private int[] removingReplicas = Replicas.NONE;
+ private int[] addingReplicas = Replicas.NONE;
+ private Integer leader;
+ private LeaderRecoveryState leaderRecoveryState;
+ private Integer leaderEpoch;
+ private Integer partitionEpoch;
+
+ public Builder setReplicas(int[] replicas) {
+ this.replicas = replicas;
+ return this;
+ }
+
+ public Builder setIsr(int[] isr) {
+ this.isr = isr;
+ return this;
+ }
+
+ public Builder setRemovingReplicas(int[] removingReplicas) {
+ this.removingReplicas = removingReplicas;
+ return this;
+ }
+
+ public Builder setAddingReplicas(int[] addingReplicas) {
+ this.addingReplicas = addingReplicas;
+ return this;
+ }
+
+ public Builder setLeader(Integer leader) {
+ this.leader = leader;
+ return this;
+ }
+
+ public Builder setLeaderRecoveryState(LeaderRecoveryState leaderRecoveryState) {
+ this.leaderRecoveryState = leaderRecoveryState;
+ return this;
+ }
+
+ public Builder setLeaderEpoch(Integer leaderEpoch) {
+ this.leaderEpoch = leaderEpoch;
+ return this;
+ }
+
+ public Builder setPartitionEpoch(Integer partitionEpoch) {
+ this.partitionEpoch = partitionEpoch;
+ return this;
+ }
+
+ public PartitionRegistration build() {
+ if (replicas == null) {
+ throw new IllegalStateException("You must set replicas.");
+ } else if (isr == null) {
+ throw new IllegalStateException("You must set isr.");
+ } else if (removingReplicas == null) {
+ throw new IllegalStateException("You must set removing replicas.");
+ } else if (addingReplicas == null) {
+ throw new IllegalStateException("You must set adding replicas.");
+ } else if (leader == null) {
+ throw new IllegalStateException("You must set leader.");
+ } else if (leaderRecoveryState == null) {
+ throw new IllegalStateException("You must set leader recovery state.");
Review Comment:
I should have mentioned in my previous review but how about having a default of this state too? The default should be `RECOVERED`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org