[GitHub] [ignite-3] kgusakov commented on a diff in pull request #1348: IGNITE-18155 Add learners to RaftGroupService#changePeersAsync

[GitBox <gi...@apache.org>]

kgusakov commented on code in PR #1348:
URL: https://github.com/apache/ignite-3/pull/1348#discussion_r1027980124


##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/PartitionMoverTest.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.table.distributed;
+
+import static java.util.concurrent.CompletableFuture.completedFuture;
+import static java.util.concurrent.CompletableFuture.failedFuture;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureExceptionMatcher.willThrowWithCauseOrSuppressed;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.internal.raft.RaftGroupServiceImpl;
+import org.apache.ignite.internal.util.IgniteSpinBusyLock;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.raft.client.service.RaftGroupService;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for the {@link PartitionMover} class.
+ */
+class PartitionMoverTest {
+    /**
+     * Tests that {@link RaftGroupServiceImpl#changePeersAsync} was retried after some exceptions.
+     */
+    @Test
+    public void testChangePeersAsyncRetryLogic() {
+        RaftGroupService raftService = mock(RaftGroupService.class);
+
+        when(raftService.changePeersAsync(any(), any(), anyLong()))
+                .thenReturn(failedFuture(new RuntimeException()))
+                .thenReturn(failedFuture(new IOException()))
+                .thenReturn(completedFuture(null));
+
+        var partitionMover = new PartitionMover(new IgniteSpinBusyLock(), () -> raftService);
+
+        assertThat(partitionMover.movePartition(List.of(), List.of(), 1), willCompleteSuccessfully());
+
+        verify(raftService, times(3)).changePeersAsync(any(), any(), anyLong());

Review Comment:
   We need to check, that it was not any `changePeersAsync`, but the one with the correct peers and learners list and leader term.



##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/PartitionMover.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.table.distributed;
+
+import static org.apache.ignite.internal.utils.RebalanceUtil.recoverable;
+import static org.apache.ignite.lang.ErrorGroups.Common.NODE_STOPPING_ERR;
+
+import java.util.Collection;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Function;
+import java.util.function.Supplier;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.apache.ignite.internal.util.IgniteSpinBusyLock;
+import org.apache.ignite.lang.IgniteInternalException;
+import org.apache.ignite.lang.NodeStoppingException;
+import org.apache.ignite.raft.client.Peer;
+import org.apache.ignite.raft.client.service.RaftGroupService;
+
+/**
+ * Class for moving partitions.
+ */
+public class PartitionMover {

Review Comment:
   Didn't understand how this change connected with the topic of PR. But as I understand from another PRs - refactoring of the random code pieces as a part of PR is your strong position :) It's just makes any review slower and harder for reviewer, just a note.



##########
modules/raft/src/test/java/org/apache/ignite/raft/jraft/rpc/impl/cli/ChangePeersAsyncRequestProcessorTest.java:
##########
@@ -22,22 +22,26 @@
 
 import java.util.List;
 import org.apache.ignite.raft.jraft.Closure;
-import org.apache.ignite.raft.jraft.JRaftUtils;
 import org.apache.ignite.raft.jraft.Node;
 import org.apache.ignite.raft.jraft.Status;
+import org.apache.ignite.raft.jraft.conf.Configuration;
 import org.apache.ignite.raft.jraft.entity.PeerId;
 import org.apache.ignite.raft.jraft.rpc.CliRequests.ChangePeersAsyncRequest;
 import org.apache.ignite.raft.jraft.rpc.CliRequests.ChangePeersAsyncResponse;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
 
 public class ChangePeersAsyncRequestProcessorTest extends AbstractCliRequestProcessorTest<ChangePeersAsyncRequest>{
+    private static final List<String> FOLLOWERS = List.of("follower1", "follower2");
+    private static final List<String> LEARNERS = List.of("learner1", "learner2", "learner3");
+
     @Override
     public ChangePeersAsyncRequest createRequest(String groupId, PeerId peerId) {
         return msgFactory.changePeersAsyncRequest()
                 .groupId(groupId)
                 .leaderId(peerId.toString())
-                .newPeersList(List.of("localhost:8084", "localhost:8085"))
+                .newPeersList(FOLLOWERS)

Review Comment:
   It looks like we have some mess in naming. Every raft group (according to JRaft naming) has the members = peers + learners. Follower is a state of the member, other states: candidate, leader. So, each active member is a follower. I'd prefer to replace FOLLOWERS by PEERS



##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/cli/ChangePeersAsyncRequestProcessor.java:
##########
@@ -65,6 +67,17 @@ protected Message processRequest0(final CliRequestContext ctx, final ChangePeers
             }
         }
 
+        for (final String peerIdStr : request.newLearnersList()) {
+            final PeerId peer = new PeerId();
+            if (peer.parse(peerIdStr)) {
+                conf.addLearner(peer);
+            }
+            else {
+                return RaftRpcFactory.DEFAULT //
+                        .newResponse(msgFactory(), RaftError.EINVAL, "Fail to parse peer id %s", peerIdStr);

Review Comment:
   Could you rename variables to `learner*` too?



##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/cli/ChangePeersAsyncRequestProcessor.java:
##########
@@ -65,6 +67,17 @@ protected Message processRequest0(final CliRequestContext ctx, final ChangePeers
             }
         }
 
+        for (final String peerIdStr : request.newLearnersList()) {
+            final PeerId peer = new PeerId();
+            if (peer.parse(peerIdStr)) {
+                conf.addLearner(peer);
+            }
+            else {
+                return RaftRpcFactory.DEFAULT //
+                        .newResponse(msgFactory(), RaftError.EINVAL, "Fail to parse peer id %s", peerIdStr);

Review Comment:
   ```suggestion
                           .newResponse(msgFactory(), RaftError.EINVAL, "Fail to parse learner id %s", peerIdStr);
   ```



-- 
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-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org