IGNITE-24206 Add index and term to RaftGroupConfiguration #5083
Conversation
alievmirza
changed the title
| @IgniteToStringInclude | ||
| private final List<String> peers; | ||
| @IgniteToStringInclude | ||
| private final List<String> learners; | ||
|
|
||
|
|
||
|
|
| @@ -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.
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.
| @@ -61,10 +65,12 @@ void serializationAndDeserializationWithNulls() { | |||
|
|
|||
| @Test | |||
| void v1CanBeDeserialized() { | |||
There was a problem hiding this comment.
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 (v2CanBeDeserialized())
| @@ -57,6 +57,10 @@ public interface SnapshotMeta extends Message { | |||
|
|
|||
| long lastIncludedTerm(); | |||
|
|
|||
| long cfgIndex(); | |||
There was a problem hiding this comment.
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, PartitionSnapshotMeta seems to be a more suitable place to add these fields.
| @@ -32,11 +32,16 @@ | |||
| public class RaftGroupConfiguration implements Serializable { | |||
There was a problem hiding this comment.
This class now seems to be identical to CommittedConfiguration. Do we need to keep both?
|
|
||
| assertTrue(cluster.stop(newPeer.getPeerId())); | ||
|
|
||
| // apply something more |
There was a problem hiding this comment.
| // apply something more | |
| // Apply something more. |
| assertEquals(3, cluster.getFsms().size()); | ||
| for (MockStateMachine fsm : cluster.getFsms()) | ||
| assertEquals(20, fsm.getLogs().size(), fsm.getPeerId().toString()); |
There was a problem hiding this comment.
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
| metaTerm.set(meta.cfgTerm()); | ||
| metaIndex.set(meta.cfgIndex()); |
There was a problem hiding this comment.
Can any of 3 nodes write its index+term to atomics or just the leader? How about restricting this explicitly by peerId?
There was a problem hiding this comment.
If we only need indexes and terms from the leader, then why do we need other nodes? Why one of them is stopped?
|
|
||
| // 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.
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?
| @@ -3506,6 +3507,102 @@ public void onRawConfigurationCommitted(ConfigurationEntry conf) { | |||
| ); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testIndexAndTermOfCfgArePropagatedToSnapshotMeta() throws Exception { | |||
There was a problem hiding this comment.
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...
https://issues.apache.org/jira/browse/IGNITE-24206
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
The Review Checklist
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Notes