-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-24206 Add index and term to RaftGroupConfiguration #5083
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,11 +32,16 @@ | |
public class RaftGroupConfiguration implements Serializable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class now seems to be identical to |
||
private static final long serialVersionUID = 0; | ||
|
||
private final long index; | ||
private final long term; | ||
|
||
@IgniteToStringInclude | ||
private final List<String> peers; | ||
@IgniteToStringInclude | ||
private final List<String> learners; | ||
|
||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extra blank line |
||
@IgniteToStringInclude | ||
private final @Nullable List<String> oldPeers; | ||
@IgniteToStringInclude | ||
|
@@ -46,11 +51,16 @@ public class RaftGroupConfiguration implements Serializable { | |
* Creates a new instance. | ||
*/ | ||
public RaftGroupConfiguration( | ||
long index, | ||
long term, | ||
Collection<String> peers, | ||
Collection<String> learners, | ||
@Nullable Collection<String> oldPeers, | ||
@Nullable Collection<String> oldLearners | ||
|
||
) { | ||
this.index = index; | ||
this.term = term; | ||
this.peers = List.copyOf(peers); | ||
this.learners = List.copyOf(learners); | ||
this.oldPeers = oldPeers == null ? null : List.copyOf(oldPeers); | ||
|
@@ -62,13 +72,23 @@ public RaftGroupConfiguration( | |
*/ | ||
public static RaftGroupConfiguration fromCommittedConfiguration(CommittedConfiguration config) { | ||
return new RaftGroupConfiguration( | ||
config.index(), | ||
config.term(), | ||
config.peers(), | ||
config.learners(), | ||
config.oldPeers(), | ||
config.oldLearners() | ||
); | ||
} | ||
|
||
public long index() { | ||
return index; | ||
} | ||
|
||
public long term() { | ||
return term; | ||
} | ||
|
||
/** | ||
* Returns peers of the current configuration. | ||
* | ||
|
@@ -121,7 +141,7 @@ public boolean equals(Object o) { | |
return false; | ||
} | ||
RaftGroupConfiguration that = (RaftGroupConfiguration) o; | ||
return Objects.equals(peers, that.peers) && Objects.equals(learners, that.learners) | ||
return index == that.index && term == that.term && Objects.equals(peers, that.peers) && Objects.equals(learners, that.learners) | ||
&& Objects.equals(oldPeers, that.oldPeers) && Objects.equals(oldLearners, that.oldLearners); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,8 @@ public class RaftGroupConfigurationSerializer extends VersionedSerializer<RaftGr | |
|
||
@Override | ||
protected void writeExternalData(RaftGroupConfiguration config, IgniteDataOutput out) throws IOException { | ||
out.writeLong(config.index()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR is not going to go to release 3.0 and this is a breaking change. Let's create second version of the serializer. Another option is to agree on the mailing list to include this issue to the release. |
||
out.writeLong(config.term()); | ||
writeStringList(config.peers(), out); | ||
writeStringList(config.learners(), out); | ||
writeNullableStringList(config.oldPeers(), out); | ||
|
@@ -57,12 +59,14 @@ private static void writeNullableStringList(@Nullable List<String> strings, Igni | |
|
||
@Override | ||
protected RaftGroupConfiguration readExternalData(byte protoVer, IgniteDataInput in) throws IOException { | ||
long index = in.readLong(); | ||
long term = in.readLong(); | ||
List<String> peers = readStringList(in); | ||
List<String> learners = readStringList(in); | ||
List<String> oldPeers = readNullableStringList(in); | ||
List<String> oldLearners = readNullableStringList(in); | ||
|
||
return new RaftGroupConfiguration(peers, learners, oldPeers, oldLearners); | ||
return new RaftGroupConfiguration(index, term, peers, learners, oldPeers, oldLearners); | ||
} | ||
|
||
private static List<String> readStringList(IgniteDataInput in) throws IOException { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,8 @@ class RaftGroupConfigurationSerializerTest { | |
@Test | ||
void serializationAndDeserializationWithoutNulls() { | ||
RaftGroupConfiguration originalConfig = new RaftGroupConfiguration( | ||
13L, | ||
37L, | ||
List.of("peer1", "peer2"), | ||
List.of("learner1", "learner2"), | ||
List.of("old-peer1", "old-peer2"), | ||
|
@@ -47,6 +49,8 @@ void serializationAndDeserializationWithoutNulls() { | |
@Test | ||
void serializationAndDeserializationWithNulls() { | ||
RaftGroupConfiguration originalConfig = new RaftGroupConfiguration( | ||
13L, | ||
37L, | ||
List.of("peer1", "peer2"), | ||
List.of("learner1", "learner2"), | ||
null, | ||
|
@@ -61,10 +65,12 @@ void serializationAndDeserializationWithNulls() { | |
|
||
@Test | ||
void v1CanBeDeserialized() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this PR is not going to be included in the 3.0 release, then changes to this test need to be reverted and another test needs to be added ( |
||
byte[] bytes = Base64.getDecoder().decode("Ae++QwMGcGVlcjEGcGVlcjIDCWxlYXJuZXIxCWxlYXJuZXIyAwpvbGQtcGVlcjEKb2xkLXBlZXIyAw1vbGQ" | ||
+ "tbGVhcm5lcjENb2xkLWxlYXJuZXIy"); | ||
byte[] bytes = Base64.getDecoder().decode("Ae++Qw0AAAAAAAAAJQAAAAAAAAADBnBlZXIxBnBlZXIyAwlsZWFybmVyMQlsZWFybmVyMgMKb2xkLXBl" | ||
+ "ZXIxCm9sZC1wZWVyMgMNb2xkLWxlYXJuZXIxDW9sZC1sZWFybmVyMg=="); | ||
RaftGroupConfiguration restoredConfig = VersionedSerialization.fromBytes(bytes, serializer); | ||
|
||
assertThat(restoredConfig.index(), is(13L)); | ||
assertThat(restoredConfig.term(), is(37L)); | ||
assertThat(restoredConfig.peers(), is(List.of("peer1", "peer2"))); | ||
assertThat(restoredConfig.learners(), is(List.of("learner1", "learner2"))); | ||
assertThat(restoredConfig.oldPeers(), is(List.of("old-peer1", "old-peer2"))); | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -126,6 +126,7 @@ | |||||||||
import org.apache.ignite.raft.jraft.entity.EnumOutter; | ||||||||||
import org.apache.ignite.raft.jraft.entity.NodeId; | ||||||||||
import org.apache.ignite.raft.jraft.entity.PeerId; | ||||||||||
import org.apache.ignite.raft.jraft.entity.RaftOutter.SnapshotMeta; | ||||||||||
import org.apache.ignite.raft.jraft.entity.Task; | ||||||||||
import org.apache.ignite.raft.jraft.entity.UserLog; | ||||||||||
import org.apache.ignite.raft.jraft.error.LogIndexOutOfBoundsException; | ||||||||||
|
@@ -3506,6 +3507,102 @@ public void onRawConfigurationCommitted(ConfigurationEntry conf) { | |||||||||
); | ||||||||||
} | ||||||||||
|
||||||||||
@Test | ||||||||||
public void testIndexAndTermOfCfgArePropagatedToSnapshotMeta() throws Exception { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The scenario is pretty lengthy and complex. Could you please elaborate on it in the javadoc? What it tests for, what the scenario is, what verifications are made... |
||||||||||
TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT); | ||||||||||
|
||||||||||
AtomicLong term = new AtomicLong(-1); | ||||||||||
AtomicLong index = new AtomicLong(-1); | ||||||||||
Comment on lines
+3514
to
+3515
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
AtomicLong metaTerm = new AtomicLong(-1); | ||||||||||
AtomicLong metaIndex = new AtomicLong(-1); | ||||||||||
Comment on lines
+3517
to
+3518
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
cluster = new TestCluster( | ||||||||||
"testIndexAndTermOfCfgArePropagatedToSnapshotMeta", | ||||||||||
dataPath, | ||||||||||
Collections.singletonList(peer0), | ||||||||||
new LinkedHashSet<>(), | ||||||||||
ELECTION_TIMEOUT_MILLIS, | ||||||||||
(peerId, opts) -> { | ||||||||||
opts.setFsm(new MockStateMachine(peerId) { | ||||||||||
@Override | ||||||||||
public boolean onSnapshotLoad(SnapshotReader reader) { | ||||||||||
SnapshotMeta meta = reader.load(); | ||||||||||
|
||||||||||
metaTerm.set(meta.cfgTerm()); | ||||||||||
metaIndex.set(meta.cfgIndex()); | ||||||||||
Comment on lines
+3532
to
+3533
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can any of 3 nodes write its index+term to atomics or just the leader? How about restricting this explicitly by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we only need indexes and terms from the leader, then why do we need other nodes? Why one of them is stopped? |
||||||||||
|
||||||||||
return super.onSnapshotLoad(reader); | ||||||||||
} | ||||||||||
|
||||||||||
@Override | ||||||||||
public void onRawConfigurationCommitted(ConfigurationEntry conf) { | ||||||||||
term.set(conf.getId().getTerm()); | ||||||||||
index.set(conf.getId().getIndex()); | ||||||||||
System.out.println("Index + term = " + index + " " + term); | ||||||||||
super.onRawConfigurationCommitted(conf); | ||||||||||
} | ||||||||||
}); | ||||||||||
}, | ||||||||||
testInfo | ||||||||||
); | ||||||||||
|
||||||||||
assertTrue(cluster.start(peer0)); | ||||||||||
|
||||||||||
Node leader = cluster.waitAndGetLeader(); | ||||||||||
|
||||||||||
assertEquals(1, term.get()); | ||||||||||
assertEquals(1, index.get()); | ||||||||||
|
||||||||||
TestPeer newPeer = new TestPeer(testInfo, TestUtils.INIT_PORT + 1); | ||||||||||
TestPeer otherPeer = new TestPeer(testInfo, TestUtils.INIT_PORT + 2); | ||||||||||
|
||||||||||
assertTrue(cluster.start(newPeer, false, 300)); | ||||||||||
assertTrue(cluster.start(otherPeer, false, 300)); | ||||||||||
|
||||||||||
// Wait until new node node sees every other node, otherwise | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
// changePeersAndLearnersAsync can fail. | ||||||||||
waitForTopologyOnEveryNode(1, cluster); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we seem to wait till every node sees at least 1 node in its physical topology. But physical topology includes the node itself, so how is it about seeing other nodes, let alone seeing each other node? |
||||||||||
|
||||||||||
SynchronizedClosure done = new SynchronizedClosure(); | ||||||||||
leader.changePeersAndLearnersAsync( | ||||||||||
new Configuration(List.of(peer0.getPeerId(), newPeer.getPeerId(), otherPeer.getPeerId()), List.of()), leader.getCurrentTerm(), | ||||||||||
done | ||||||||||
); | ||||||||||
|
||||||||||
assertEquals(done.await(), Status.OK()); | ||||||||||
|
||||||||||
assertTrue(waitForCondition(() -> cluster.getLeader().listAlivePeers().contains(newPeer.getPeerId()), 10_000)); | ||||||||||
|
||||||||||
assertTrue(cluster.stop(newPeer.getPeerId())); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we stop |
||||||||||
|
||||||||||
// apply something more | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
sendTestTaskAndWait(leader); | ||||||||||
triggerLeaderSnapshot(cluster, leader); | ||||||||||
|
||||||||||
sendTestTaskAndWait(leader, 10); | ||||||||||
|
||||||||||
triggerLeaderSnapshot(cluster, leader, 2); | ||||||||||
|
||||||||||
//restart follower. | ||||||||||
cluster.clean(newPeer.getPeerId()); | ||||||||||
assertTrue(cluster.start(newPeer, false, 300)); | ||||||||||
|
||||||||||
cluster.ensureSame(); | ||||||||||
|
||||||||||
assertEquals(3, cluster.getFsms().size()); | ||||||||||
for (MockStateMachine fsm : cluster.getFsms()) | ||||||||||
assertEquals(20, fsm.getLogs().size(), fsm.getPeerId().toString()); | ||||||||||
Comment on lines
+3593
to
+3595
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are these assertions needed? They don't seem to relate to the thing the test is testing, so they just serve as an obstacle when trying to understand what happens here |
||||||||||
|
||||||||||
// Leader hasn't been changed, term must stay the same | ||||||||||
assertEquals(1, term.get()); | ||||||||||
// idx_2 == joint consensus, idx_3 is expected final cfg | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm not mistaken, the scenario starts with having just [A] as voting set and then expands it to [ABC]. It looks like joint config is the final configuration. Does it really require 2 steps? |
||||||||||
assertEquals(3, index.get()); | ||||||||||
|
||||||||||
assertEquals(1, metaTerm.get()); | ||||||||||
assertEquals(3, metaIndex.get()); | ||||||||||
} | ||||||||||
|
||||||||||
@Test | ||||||||||
public void testOnNewPeersConfigurationAppliedIsNotCalledAfterResetPeers() throws Exception { | ||||||||||
TestPeer peer0 = new TestPeer(testInfo, TestUtils.INIT_PORT); | ||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -603,6 +603,8 @@ private void doSnapshotSave(final SaveSnapshotClosure done) { | |
} | ||
|
||
SnapshotMetaBuilder metaBuilder = msgFactory.snapshotMeta() | ||
.cfgIndex(confEntry.getId().getIndex()) | ||
.cfgTerm(confEntry.getId().getTerm()) | ||
Comment on lines
+606
to
+607
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These 2 fields are only needed for partitions, and for partitions we don't save snapshot files. Hence, a question: do we really need to fill them here? |
||
.lastIncludedIndex(lastAppliedIndex) | ||
.lastIncludedTerm(this.lastAppliedTerm) | ||
.peersList(confEntry.getConf().getPeers().stream().map(Object::toString).collect(toList())) | ||
|
@@ -700,7 +702,7 @@ private void doSnapshotLoad(final LoadSnapshotClosure done) { | |
// so we have to protect from this. In production, these methods never return null. | ||
if (meta.peersList() != null && meta.learnersList() != null) { | ||
ConfigurationEntry configurationEntry = new ConfigurationEntry( | ||
snapshotId.copy(), | ||
new LogId(meta.cfgIndex(), meta.cfgTerm()), | ||
new Configuration( | ||
meta.peersList().stream().map(PeerId::parsePeer).collect(toList()), | ||
meta.learnersList().stream().map(PeerId::parsePeer).collect(toList()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,10 @@ public interface SnapshotMeta extends Message { | |
|
||
long lastIncludedTerm(); | ||
|
||
long cfgIndex(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need these fields for all raft groups or just for partitions (or for partitions + metastorage)? If not for all of them, then probably they don't need to be added here as they are not part of the Raft protocol. If only partitions need them, |
||
|
||
long cfgTerm(); | ||
|
||
@Nullable Collection<String> peersList(); | ||
|
||
@Nullable Collection<String> oldPeersList(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Metastorage, these fields are not needed, so now it looks like we are just conforming to a ceremony. It would be nice if we could avoid using these new 2 fields here.