IGNITE-23870 Implement a method to receive a chain of topology changes #4966
Conversation
| * | ||
| * @param partId Unique identifier of a partition. | ||
| * @return Key for a partition. | ||
| * @see <a href="https://github.com/apache/ignite-3/blob/main/modules/table/tech-notes/rebalance.md">Rebalance documentation</a> |
There was a problem hiding this comment.
There is no information about this key in this doc. I would change description to something like "Key for the graceful restart in HA mode", also we can add link to IEP
There was a problem hiding this comment.
Also, do we need the same mechanism in ZoneRebalanceUtil for the purposes of collocation?
There was a problem hiding this comment.
Changed the javadoc.
For ZoneRebalanceUtil please raise a separate jira
...c/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChainSerializer.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChainSerializer.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChainSerializer.java
Outdated
Show resolved
Hide resolved
...org/apache/ignite/internal/distributionzones/rebalance/RebalanceRaftGroupEventsListener.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'd rather add some unit tests, however integration ones, that you wrote cover major scenarious. Anyway, if you will decide to add some unit tests, please don't do it with given ticket and please postpone the active until you will return from holidays.
Generally PR is good.
...org/apache/ignite/internal/distributionzones/rebalance/RebalanceRaftGroupEventsListener.java
Show resolved
Hide resolved
...ibution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChain.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** Chain of assignments. */ | ||
| @IgniteToStringInclude | ||
| private final List<Assignments> chain; |
There was a problem hiding this comment.
I though that you have a wrapper for Assignments that we may use to store configurationIndex in. If not - not a problem, we will add one within https://issues.apache.org/jira/browse/IGNITE-24106.
There was a problem hiding this comment.
Let's do it in IGNITE-24106.
...ibution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChain.java
Show resolved
Hide resolved
| @@ -1234,6 +1242,295 @@ void testTwoPhaseResetOnEmptyNodes() throws Exception { | |||
| assertPlannedAssignments(node0, partId, assignments13); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Could you please run given and other tests in the class with @RepeatedTest locally in order to verify that they are stable?
|
|
||
| @Test | ||
| @ZoneParams(nodes = 7, replicas = 7, partitions = 1, consistencyMode = ConsistencyMode.HIGH_AVAILABILITY) | ||
| void testSecondResetRewritesFirst() throws Exception { |
There was a problem hiding this comment.
The name is misleading. second phaze of reset should rewrite first one, on the other hand second reset(full) should append to the first one.
There was a problem hiding this comment.
what about testSecondResetRewritesUnfinishedFirstPhaseReset?
| @Retention(RetentionPolicy.RUNTIME) | ||
| @interface ZoneParams { | ||
| int replicas(); | ||
|
|
||
| int partitions(); | ||
|
|
||
| int nodes() default INITIAL_NODES; | ||
|
|
||
| ConsistencyMode consistencyMode() default ConsistencyMode.STRONG_CONSISTENCY; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd say that we miss:
- Verification that graceful change will rewrite chain after force resets.
- Verification that !HA zone has empty chain.
| /** | ||
| * {@link VersionedSerializer} for {@link AssignmentsChain} instances. | ||
| */ | ||
| public class AssignmentsChainSerializer extends VersionedSerializer<AssignmentsChain> { |
There was a problem hiding this comment.
One thing that I forgot to mention: please add unit tests for this class, including one that will check that serialization made with the current code can be later deserialized (there are examples named approximately v1CanBeDeserialized() in each XXXSerializerTest)
https://issues.apache.org/jira/browse/IGNITE-23870