jsancio commented on code in PR #17728:
URL: https://github.com/apache/kafka/pull/17728#discussion_r1888921311
##########
raft/src/main/java/org/apache/kafka/raft/internals/VoterSetHistory.java:
##########
@@ -47,24 +47,6 @@ public final class VoterSetHistory {
* @throws IllegalArgumentException if the offset is not greater than all
previous offsets
*/
public void addAt(long offset, VoterSet voters) {
- Optional<LogHistory.Entry<VoterSet>> lastEntry =
votersHistory.lastEntry();
- if (lastEntry.isPresent() && lastEntry.get().offset() >= 0) {
- // If the last voter set comes from the replicated log then the
majorities must overlap.
- // This ignores the static voter set and the bootstrapped voter
set since they come from
- // the configuration and the KRaft leader never guaranteed that
they are the same across
- // 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",
- lastVoterSet,
- voters
- )
- );
Review Comment:
Instead of removing this check and throwing an exception. We can instead log
the same message at INFO.
You can create the `Logger` from passing the `LogContext` to this type's
constructor.
##########
raft/src/main/java/org/apache/kafka/raft/VoterSet.java:
##########
@@ -270,26 +269,6 @@ public VotersRecord toVotersRecord(short version) {
.setVoters(voterRecordVoters);
}
- /**
- * Determines if two sets of voters have an overlapping majority.
- *
- * An overlapping majority means that for all majorities in {@code this}
set of voters and for
- * all majority in {@code that} set of voters, they have at least one
voter in common.
- *
- * If this function returns true, it means that if one of the set of
voters commits an offset,
- * the other set of voters cannot commit a conflicting offset.
- *
- * @param that the other voter set to compare
- * @return true if they have an overlapping majority, false otherwise
- */
- public boolean hasOverlappingMajority(VoterSet that) {
- Set<ReplicaKey> thisReplicaKeys = voterKeys();
- Set<ReplicaKey> thatReplicaKeys = that.voterKeys();
-
- if (Utils.diff(HashSet::new, thisReplicaKeys, thatReplicaKeys).size()
> 1) return false;
- return Utils.diff(HashSet::new, thatReplicaKeys,
thisReplicaKeys).size() <= 1;
- }
Review Comment:
See my other message but let's keep this functionality.
##########
raft/src/test/java/org/apache/kafka/raft/internals/VoterSetHistoryTest.java:
##########
@@ -122,55 +143,6 @@ void testBootstrapAddAt() {
assertEquals(Optional.of(removedVoterSet),
votersHistory.valueAtOrBefore(200));
}
- @Test
- void testAddAtNonOverlapping() {
- VoterSetHistory votersHistory = new VoterSetHistory(VoterSet.empty());
-
- Map<Integer, VoterSet.VoterNode> voterMap =
VoterSetTest.voterMap(IntStream.of(1, 2, 3), true);
- VoterSet voterSet = VoterSet.fromMap(new HashMap<>(voterMap));
-
- // Add a starting voter to the history
- votersHistory.addAt(100, voterSet);
-
- // Remove voter so that it doesn't overlap
- VoterSet nonoverlappingRemovedSet = voterSet
- .removeVoter(voterMap.get(1).voterKey()).get()
- .removeVoter(voterMap.get(2).voterKey()).get();
-
- assertThrows(
- IllegalArgumentException.class,
- () -> votersHistory.addAt(200, nonoverlappingRemovedSet)
- );
- assertEquals(voterSet, votersHistory.lastValue());
-
-
- // Add voters so that it doesn't overlap
- VoterSet nonoverlappingAddSet = voterSet
- .addVoter(VoterSetTest.voterNode(4, true)).get()
- .addVoter(VoterSetTest.voterNode(5, true)).get();
-
- assertThrows(
- IllegalArgumentException.class,
- () -> votersHistory.addAt(200, nonoverlappingAddSet)
- );
- assertEquals(voterSet, votersHistory.lastValue());
- }
-
- @Test
- void testNonoverlappingFromStaticVoterSet() {
- Map<Integer, VoterSet.VoterNode> voterMap =
VoterSetTest.voterMap(IntStream.of(1, 2, 3), true);
- VoterSet staticVoterSet = VoterSet.fromMap(new HashMap<>(voterMap));
- VoterSetHistory votersHistory = new VoterSetHistory(VoterSet.empty());
-
- // Remove voter so that it doesn't overlap
- VoterSet nonoverlappingRemovedSet = staticVoterSet
- .removeVoter(voterMap.get(1).voterKey()).get()
- .removeVoter(voterMap.get(2).voterKey()).get();
-
- votersHistory.addAt(100, nonoverlappingRemovedSet);
- assertEquals(nonoverlappingRemovedSet, votersHistory.lastValue());
- }
Review Comment:
This test is still correct after your change and after my suggestions. Let's
keep this change.
##########
raft/src/test/java/org/apache/kafka/raft/VoterSetTest.java:
##########
@@ -258,76 +258,6 @@ void testRecordRoundTrip() {
assertEquals(voterSet,
VoterSet.fromVotersRecord(voterSet.toVotersRecord((short) 0)));
}
- @Test
- void testOverlappingMajority() {
Review Comment:
Let's keep all of these tests now that we are keeping the
`hasOverlappingMajority` method.
##########
raft/src/test/java/org/apache/kafka/raft/internals/VoterSetHistoryTest.java:
##########
@@ -89,6 +89,27 @@ void testAddAt() {
assertEquals(Optional.empty(), votersHistory.valueAtOrBefore(99));
assertEquals(Optional.of(addedVoterSet),
votersHistory.valueAtOrBefore(199));
assertEquals(Optional.of(removedVoterSet),
votersHistory.valueAtOrBefore(200));
+
+ // 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(300, addedMultipleVoters);
+
+ assertEquals(addedMultipleVoters, votersHistory.lastValue());
+ assertEquals(Optional.of(removedVoterSet),
votersHistory.valueAtOrBefore(299));
+ assertEquals(Optional.of(addedMultipleVoters),
votersHistory.valueAtOrBefore(300));
+
+ voterMap.remove(4);
+ voterMap.remove(5);
+
+ VoterSet removedMultipleVoters = VoterSet.fromMap(new
HashMap<>(voterMap));
+ votersHistory.addAt(400, removedMultipleVoters);
+
+ assertEquals(removedMultipleVoters, votersHistory.lastValue());
+ assertEquals(Optional.of(addedMultipleVoters),
votersHistory.valueAtOrBefore(399));
+ assertEquals(Optional.of(removedMultipleVoters),
votersHistory.valueAtOrBefore(400));
}
Review Comment:
The test looks good but let's put it in a separate test that is checking
this case specifically. This test is already getting hard to read.
--
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]
