denis-chudov commented on code in PR #5244: URL: https://github.com/apache/ignite-3/pull/5244#discussion_r1970080393
########## modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsQueue.java: ########## @@ -0,0 +1,138 @@ +/* + * 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.Arrays; +import java.util.Collection; +import java.util.Deque; +import java.util.Iterator; +import java.util.LinkedList; +import java.util.Objects; +import org.apache.ignite.internal.tostring.IgniteToStringInclude; +import org.apache.ignite.internal.tostring.S; +import org.apache.ignite.internal.versioned.VersionedSerialization; +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; + +/** + * Class that encapsulates a queue with consequent configuration switches + * e.g. promoting a learner to a peer or demoting a peer to a learner in following steps: + * <ul> + * <li> node removed from configuration + * <li> node added to configuration with different type (peer or learner) + * </ul> + */ +public class AssignmentsQueue implements Iterable<Assignments> { + + @IgniteToStringInclude + private final Deque<Assignments> queue; + + /** Constructor. */ + public AssignmentsQueue(Assignments... assignments) { + this(Arrays.asList(assignments)); + } + + /** Constructor. */ + AssignmentsQueue(Collection<Assignments> assignments) { + this.queue = new LinkedList<>(assignments); + } + + /** + * Retrieves and removes the head of this queue, or returns {@code Assignments.EMPTY} if this queue is empty. + * + * @return the head of this queue, or {@code Assignments.EMPTY} if this queue is empty + */ + public @Nullable Assignments poll() { + return queue.poll(); + } Review Comment: Javadoc is now incorrect, because the method is nullable. Why don't you want to add an assertion that the queue is not empty? It should never return null ########## modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java: ########## @@ -2211,14 +2215,14 @@ private CompletableFuture<Void> handleChangePendingAssignmentEvent( return nullCompletedFuture(); } - TablePartitionId replicaGrpId = extractTablePartitionId(pendingAssignmentsEntry.key(), PENDING_ASSIGNMENTS_PREFIX_BYTES); + TablePartitionId replicaGrpId = extractTablePartitionId(pendingAssignmentsEntry.key(), PENDING_ASSIGNMENTS_QUEUE_PREFIX_BYTES); // Stable assignments from the meta store, which revision is bounded by the current pending event. Assignments stableAssignments = stableAssignmentsGetLocally(metaStorageMgr, replicaGrpId, revision); AssignmentsChain assignmentsChain = assignmentsChainGetLocally(metaStorageMgr, replicaGrpId, revision); - Assignments pendingAssignments = Assignments.fromBytes(pendingAssignmentsEntry.value()); + Assignments pendingAssignments = AssignmentsQueue.fromBytes(pendingAssignmentsEntry.value()).poll(); Review Comment: and now NPE may be thrown on usages of `pendingAssignments`? ########## modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/AssignmentsQueue.java: ########## @@ -0,0 +1,138 @@ +/* + * 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.Arrays; +import java.util.Collection; +import java.util.Deque; +import java.util.Iterator; +import java.util.LinkedList; +import java.util.Objects; +import org.apache.ignite.internal.tostring.IgniteToStringInclude; +import org.apache.ignite.internal.tostring.S; +import org.apache.ignite.internal.versioned.VersionedSerialization; +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; + +/** + * Class that encapsulates a queue with consequent configuration switches + * e.g. promoting a learner to a peer or demoting a peer to a learner in following steps: + * <ul> + * <li> node removed from configuration + * <li> node added to configuration with different type (peer or learner) + * </ul> + */ +public class AssignmentsQueue implements Iterable<Assignments> { + + @IgniteToStringInclude + private final Deque<Assignments> queue; + + /** Constructor. */ + public AssignmentsQueue(Assignments... assignments) { + this(Arrays.asList(assignments)); + } + + /** Constructor. */ + AssignmentsQueue(Collection<Assignments> assignments) { + this.queue = new LinkedList<>(assignments); + } + + /** + * Retrieves and removes the head of this queue, or returns {@code Assignments.EMPTY} if this queue is empty. + * + * @return the head of this queue, or {@code Assignments.EMPTY} if this queue is empty + */ + public @Nullable Assignments poll() { + return queue.poll(); + } + + /** + * Retrieves, but does not remove, the last element of this queue, or returns {@code Assignments.EMPTY} if this queue is empty. + * + * @return the tail of this queue, or {@code Assignments.EMPTY} if this queue is empty + */ + public @Nullable Assignments peekLast() { Review Comment: same here ########## modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/RebalanceUtil.java: ########## @@ -200,7 +201,14 @@ public static CompletableFuture<Void> updatePendingAssignmentsKeys( boolean isNewAssignments = !tableCfgPartAssignments.equals(partAssignments); - byte[] partAssignmentsBytes = Assignments.toBytes(partAssignments, assignmentsTimestamp); + Assignments partAssignmentsPlanned = Assignments.of(partAssignments, assignmentsTimestamp); Review Comment: I still think that the naming is incorrect here, mostly thanks to `partAssignments` which is ambiguous. Let's name it `targetAssignmentSet`, which means that they can either become planned ones or the last element of pending queue, and `partAssignmentsPlanned` will be renamed to `targetAssignments` which would be more correct. Planned assignments is totally incorrect here. ########## modules/table/tech-notes/rebalance.md: ########## @@ -20,7 +20,7 @@ Every algorithm phase has the following main sections: ## New metastore keys For further steps, we should introduce some new metastore keys: - `partition.assignments.stable` - the list of peers, which process operations for a partition at the current moment. -- `partition.assignments.pending` - the list of peers, where current rebalance move the partition. +- `partition.assignments.pending` - the queue of list of peers, where current rebalance move the partition. Review Comment: ```suggestion - `partition.assignments.pending` - the queue of lists of peers, where current rebalance move the partition. Queue is needed in cases when multiple configuration switches during one rebalance are required. ``` ########## modules/table/tech-notes/rebalance.md: ########## @@ -53,7 +53,7 @@ metastoreInvoke: // atomic metastore call through multi-invoke api partition.assignments.pending = calcPartAssignments() partition.change.trigger.revision = event.revision else: - if partition.assignments.pending != calcPartAssignments + if partition.assignments.pending != partAssignmentsPendingQueue Review Comment: You replaced `calcPartAssignments` in condition but not in operation. Why? ########## modules/partition-distribution/src/main/java/org/apache/ignite/internal/partitiondistribution/PendingAssignmentsCalculator.java: ########## @@ -0,0 +1,63 @@ +/* + * 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; + +/** + * Calculates the pending assignments queue between current stable and planned assignments. + */ +public class PendingAssignmentsCalculator { + private Assignments stable; + private Assignments planned; + + private PendingAssignmentsCalculator() {} + + public static PendingAssignmentsCalculator pendingAssignmentsCalculator() { + return new PendingAssignmentsCalculator(); + } + + /** + * Current stable assignments. + */ + public PendingAssignmentsCalculator stable(Assignments stable) { + this.stable = stable; + return this; + } + + /** + * Planned assignments. + */ + public PendingAssignmentsCalculator planned(Assignments planned) { Review Comment: I still don't understand why these are planned assignments. Planned assignments is different thing that is not included into the pending queue. Let's name them `target` -- 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
