sanpwc commented on code in PR #5046:
URL: https://github.com/apache/ignite-3/pull/5046#discussion_r1922736076
##########
modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChain.java:
##########
@@ -30,15 +33,19 @@
* Contains the chain of changed assignments.
*/
public class AssignmentsChain {
+
Review Comment:
Unnecessary blank line.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceRaftGroupEventsListener.java:
##########
@@ -493,23 +499,31 @@ private static Operation handleAssignmentsChainChange(
ByteArray assignmentsChainKey,
Entry assignmentsChainEntry,
Assignments pendingAssignments,
- Assignments stableAssignments
+ Assignments stableAssignments,
+ long configurationTerm,
+ long configurationIndex
) {
// We write this key only in HA mode. See
TableManager.writeTableAssignmentsToMetastore.
if (assignmentsChainEntry.value() != null) {
AssignmentsChain updatedAssignmentsChain = updateAssignmentsChain(
AssignmentsChain.fromBytes(assignmentsChainEntry.value()),
stableAssignments,
- pendingAssignments
+ pendingAssignments,
+ configurationTerm,
+ configurationIndex
);
return put(assignmentsChainKey, updatedAssignmentsChain.toBytes());
} else {
return Operations.noop();
}
}
- private static AssignmentsChain updateAssignmentsChain(AssignmentsChain
assignmentsChain, Assignments newStable,
- Assignments pendingAssignments) {
+ private static AssignmentsChain updateAssignmentsChain(
+ AssignmentsChain assignmentsChain,
+ Assignments newStable,
+ Assignments pendingAssignments,
+ long configurationTerm,
+ long configurationIndex) {
Review Comment:
I'd rather put `) {` on a new line.
##########
modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChain.java:
##########
@@ -64,31 +71,70 @@ public AssignmentsChain replaceLast(Assignments newLast) {
* @param newLast New last link.
* @return new AssignmentsChain.
*/
- public AssignmentsChain addLast(Assignments newLast) {
+ public AssignmentsChain addLast(Assignments newLast, long
configurationTerm, long configurationIndex) {
assert !chain.isEmpty() : "Assignments chain is empty.";
- List<Assignments> newChain = new ArrayList<>(chain);
+ List<AssignmentsLink> newChain = new ArrayList<>(chain);
- newChain.add(newLast);
+ newChain.add(new AssignmentsLink(newLast, configurationTerm,
configurationIndex));
return new AssignmentsChain(newChain);
}
+
+ /**
+ * Gets the next link in the chain after the given link.
+ *
+ * @param link The link to get the next one from.
+ * @return The next link in the chain, or {@code null} if the given link
is the last one in the chain.
+ */
+ public @Nullable AssignmentsLink nextLink(AssignmentsLink link) {
Review Comment:
I didn't expect such method to be implemented. I'd rather add
AssignmentsLink.next().
##########
modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChain.java:
##########
@@ -30,15 +33,19 @@
* Contains the chain of changed assignments.
*/
public class AssignmentsChain {
+
+ private static final long DEFAULT_CONF_TERM = -1;
Review Comment:
Why do we need defaults for term and index? In other words in which cases
are we going to use them?
##########
modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChain.java:
##########
@@ -64,31 +71,70 @@ public AssignmentsChain replaceLast(Assignments newLast) {
* @param newLast New last link.
* @return new AssignmentsChain.
*/
- public AssignmentsChain addLast(Assignments newLast) {
+ public AssignmentsChain addLast(Assignments newLast, long
configurationTerm, long configurationIndex) {
assert !chain.isEmpty() : "Assignments chain is empty.";
- List<Assignments> newChain = new ArrayList<>(chain);
+ List<AssignmentsLink> newChain = new ArrayList<>(chain);
- newChain.add(newLast);
+ newChain.add(new AssignmentsLink(newLast, configurationTerm,
configurationIndex));
return new AssignmentsChain(newChain);
}
+
+ /**
+ * Gets the next link in the chain after the given link.
+ *
+ * @param link The link to get the next one from.
+ * @return The next link in the chain, or {@code null} if the given link
is the last one in the chain.
+ */
+ public @Nullable AssignmentsLink nextLink(AssignmentsLink link) {
+ int i = chain.indexOf(link);
+
+ return i < 0 || i == chain().size() - 1 ? null : chain.get(i + 1);
+ }
+
+ /**
+ * Returns the last {@link AssignmentsLink} in the chain that contains the
specified node.
+ *
+ * @param nodeConsistentId The consistent identifier of the node to search
for.
+ * @return The last {@link AssignmentsLink} that contains the node, or
{@code null} if no such link exists.
Review Comment:
I'd also add an example from the ticket here in order to illustrate what
last means.
> on input link1([A,B,C,D,E,F,G]) > link2([E,F,G]) > link3([G])
chain.lastLink(F) returns link2(E,F,G).
##########
modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsLinkSerializer.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.partitiondistribution;
+
+import java.io.IOException;
+import org.apache.ignite.internal.util.io.IgniteDataInput;
+import org.apache.ignite.internal.util.io.IgniteDataOutput;
+import org.apache.ignite.internal.versioned.VersionedSerializer;
+
+/**
+ * {@link VersionedSerializer} for {@link AssignmentsLink} instances.
+ */
+public class AssignmentsLinkSerializer extends
VersionedSerializer<AssignmentsLink> {
+
+ /** Serializer instance. */
+ public static final AssignmentsLinkSerializer INSTANCE = new
AssignmentsLinkSerializer();
+
+ @Override
+ protected void writeExternalData(AssignmentsLink link, IgniteDataOutput
out) throws IOException {
+ AssignmentsSerializer.INSTANCE.writeExternal(link.assignments(), out);
+
+ out.writeVarInt(link.configurationTerm());
Review Comment:
Why not VarLong?
##########
modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsChain.java:
##########
@@ -64,31 +71,70 @@ public AssignmentsChain replaceLast(Assignments newLast) {
* @param newLast New last link.
* @return new AssignmentsChain.
*/
- public AssignmentsChain addLast(Assignments newLast) {
+ public AssignmentsChain addLast(Assignments newLast, long
configurationTerm, long configurationIndex) {
assert !chain.isEmpty() : "Assignments chain is empty.";
- List<Assignments> newChain = new ArrayList<>(chain);
+ List<AssignmentsLink> newChain = new ArrayList<>(chain);
- newChain.add(newLast);
+ newChain.add(new AssignmentsLink(newLast, configurationTerm,
configurationIndex));
return new AssignmentsChain(newChain);
}
+
+ /**
+ * Gets the next link in the chain after the given link.
+ *
+ * @param link The link to get the next one from.
+ * @return The next link in the chain, or {@code null} if the given link
is the last one in the chain.
+ */
+ public @Nullable AssignmentsLink nextLink(AssignmentsLink link) {
+ int i = chain.indexOf(link);
+
+ return i < 0 || i == chain().size() - 1 ? null : chain.get(i + 1);
+ }
+
+ /**
+ * Returns the last {@link AssignmentsLink} in the chain that contains the
specified node.
+ *
+ * @param nodeConsistentId The consistent identifier of the node to search
for.
+ * @return The last {@link AssignmentsLink} that contains the node, or
{@code null} if no such link exists.
+ */
+ public @Nullable AssignmentsLink lastLink(String nodeConsistentId) {
+ for (int i = chain.size() - 1; i >= 0; i--) {
+ AssignmentsLink link = chain.get(i);
+ if (link.hasNode(nodeConsistentId)) {
+ return link;
+ }
+ }
+
+ return null;
+ }
Review Comment:
Did you add unit tests for lastLink() and nextLink()?
##########
modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsLink.java:
##########
@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.partitiondistribution;
+
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.ignite.internal.tostring.S;
+
+/**
+ * Represents a link in the chain of assignments.
+ *
+ * <p>An AssignmentsLink instance encapsulates a set of node assignments along
with the associated
+ * configuration term and index. This is used to keep track of changes in the
node assignments for a partition over time.
+ */
+public class AssignmentsLink {
+ private final Assignments assignments;
+ private final long configurationIndex;
+ private final long configurationTerm;
+
+ AssignmentsLink(
+ Assignments assignments,
+ long configurationTerm,
+ long configurationIndex
+ ) {
+ this.assignments = assignments;
+ this.configurationIndex = configurationIndex;
+ this.configurationTerm = configurationTerm;
+ }
+
+ public Assignments assignments() {
+ return assignments;
+ }
+
+ /**
+ * Checks if the specified node is part of the current assignments.
+ *
+ * @param nodeConsistentId The consistent identifier of the node to check.
+ * @return {@code true} if the node is present in the assignments,
otherwise {@code false}.
+ */
+
+ public boolean hasNode(String nodeConsistentId) {
+ return
assignments.nodes().stream().map(Assignment::consistentId).anyMatch(nodeId ->
nodeId.equals(nodeConsistentId));
Review Comment:
I'd rather use consistentId instead of nodeId within anyMatch. I'm about
variable naming.
##########
modules/transactions/src/integrationTest/java/org/apache/ignite/internal/disaster/ItDisasterRecoveryReconfigurationTest.java:
##########
@@ -1267,7 +1267,7 @@ void testAssignmentsChainUpdate() throws Exception {
assertStableAssignments(node0, partId, initialAssignments);
- assertAssignmentsChain(node0, partId,
AssignmentsChain.of(initialAssignments));
+ assertAssignmentsChain(node0, partId, AssignmentsChain.of(-1, -1,
initialAssignments));
Review Comment:
I didn't get why you need default term and index (there's another comment
about that above). However, If you do really need them, why not to use
`public static AssignmentsChain of(Assignments... assignments) {`
that you've introduced?
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
