jsancio commented on code in PR #17728:
URL: https://github.com/apache/kafka/pull/17728#discussion_r1893129212
##########
raft/src/main/java/org/apache/kafka/raft/internals/VoterSetHistory.java:
##########
@@ -31,9 +34,11 @@
public final class VoterSetHistory {
private final VoterSet staticVoterSet;
private final LogHistory<VoterSet> votersHistory = new
TreeMapLogHistory<>();
+ private final Logger logger;
- VoterSetHistory(VoterSet staticVoterSet) {
+ VoterSetHistory(VoterSet staticVoterSet, LogContext logContext) {
this.staticVoterSet = staticVoterSet;
+ this.logger = logContext.logger(this.getClass());
Review Comment:
In Java `this` is implicit in method calls without an object.
```java
this.logger = logContext.logger(getClass());
```
##########
raft/src/main/java/org/apache/kafka/raft/internals/VoterSetHistory.java:
##########
@@ -55,12 +60,10 @@ public void addAt(long offset, VoterSet voters) {
// all replicas.
VoterSet lastVoterSet = lastEntry.get().value();
if (!lastVoterSet.hasOverlappingMajority(voters)) {
- throw new IllegalArgumentException(
- String.format(
- "Last voter set %s doesn't have an overlapping
majority with the new voter set %s",
+ logger.info(
+ "Last voter set ({}) doesn't have an overlapping
majority with the new voter set ({})",
lastVoterSet,
voters
Review Comment:
This indentation doesn't look correct. Looks like you have extra 4 spaces.
##########
raft/src/test/java/org/apache/kafka/raft/internals/VoterSetHistoryTest.java:
##########
@@ -91,11 +92,40 @@ void testAddAt() {
assertEquals(Optional.of(removedVoterSet),
votersHistory.valueAtOrBefore(200));
}
+ @Test
+ void testAddAtNonOverlapping() {
+ Map<Integer, VoterSet.VoterNode> voterMap =
VoterSetTest.voterMap(IntStream.of(1, 2, 3), true);
+ VoterSet staticVoterSet = VoterSet.fromMap(new HashMap<>(voterMap));
+ VoterSetHistory votersHistory = new VoterSetHistory(staticVoterSet,
new LogContext());
+
+ // Assert multiple voters can be added or removed at a time
+ voterMap.put(4, VoterSetTest.voterNode(4, true));
+ voterMap.put(5, VoterSetTest.voterNode(5, true));
+
+ VoterSet addedMultipleVoters = VoterSet.fromMap(new
HashMap<>(voterMap));
+ votersHistory.addAt(100, addedMultipleVoters);
Review Comment:
This doesn't do what your comment indicates. Before this PR, voter set were
allowed to have more than one change if the last voter set was the static voter
set.
See the `testAddAtNonOverlapping` method that this PR removes to see how to
construct this case correctly.
Let's also move this method to be below the `testBootstrapAddAt` method so
that git and Github can generate a better diff.
##########
raft/src/test/java/org/apache/kafka/raft/internals/VoterSetHistoryTest.java:
##########
@@ -33,7 +34,7 @@ public final class VoterSetHistoryTest {
@Test
void testStaticVoterSet() {
VoterSet staticVoterSet =
VoterSet.fromMap(VoterSetTest.voterMap(IntStream.of(1, 2, 3), true));
- VoterSetHistory votersHistory = new VoterSetHistory(staticVoterSet);
+ VoterSetHistory votersHistory = new VoterSetHistory(staticVoterSet,
new LogContext());
Review Comment:
What do you think if we create a private method for constructing
`VoterSetHistory` in this test suite? And calling that private method when a
test needs to construct a `VoterSetHistory`?
```java
private VoterSetHistory voterSetHistory(VoterSet staticVoterSet) {
return new VoterSetHistory(staticVoterSet, new LogContext());
}
```
This comment applies to a few places in this file.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
