sanpwc commented on code in PR #6408:
URL: https://github.com/apache/ignite-3/pull/6408#discussion_r2281999248
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftService.java:
##########
@@ -281,6 +282,27 @@ private ClusterNodeMessage nodeMessage(ClusterNode node) {
.build();
}
+ /**
+ * Returns a set of consistent IDs of the learners nodes of the CMG.
+ */
+ public CompletableFuture<Set<String>> learners() {
+ Peer leader = raftService.leader();
Review Comment:
Why do you need to retrieve or refresh raft leader prior to retrieving
learners?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftService.java:
##########
@@ -281,6 +282,27 @@ private ClusterNodeMessage nodeMessage(ClusterNode node) {
.build();
}
+ /**
+ * Returns a set of consistent IDs of the learners nodes of the CMG.
Review Comment:
No, it's not. It will return known set of learners of the corresponding
raftService.
##########
modules/cluster-management/src/testFixtures/java/org/apache/ignite/internal/cluster/management/BaseItClusterManagementTest.java:
##########
@@ -44,7 +44,7 @@
public abstract class BaseItClusterManagementTest extends IgniteAbstractTest {
private static final int PORT_BASE = 10000;
- @InjectConfiguration
+ @InjectConfiguration("mock.retryTimeoutMillis = 60000")
Review Comment:
Why?
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/RpcRequestProcessor.java:
##########
@@ -52,21 +53,16 @@ public void handleRequest(final RpcContext rpcCtx, final T
request) {
if (msg != null) {
rpcCtx.sendResponse(msg);
}
+ } catch (NotLeaderException t) {
+ // It is ok if we lost leadership while a request to us was in
flight, there is no need to clutter up the log.
+ LOG.debug("handleRequest {} failed", t, request);
+ rpcCtx.sendResponse(RaftRpcFactory.DEFAULT
+ .newResponse(t.leaderId(), msgFactory, RaftError.EPERM,
t.getMessage()));
+ } catch (Throwable t) {
+ LOG.error("handleRequest {} failed", t, request);
+ rpcCtx.sendResponse(RaftRpcFactory.DEFAULT
+ .newResponse(msgFactory, RaftError.UNKNOWN.getNumber(),
"handleRequest internal error"));
}
- catch (final Throwable t) {
- if (isIgnorable(t)) {
- LOG.debug("handleRequest {} failed", t, request);
- } else {
- LOG.error("handleRequest {} failed", t, request);
- }
- rpcCtx.sendResponse(RaftRpcFactory.DEFAULT //
Review Comment:
Do you mean that prior to your changes in case of NotLeaderException we've
responded with "handleRequest internal error" that triggers request retry to
the same node?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -957,18 +960,39 @@ private void onLogicalTopologyChanged(long term) {
// If the future is not here yet, this means we are still starting, so
learners will be updated after start
// (if we happen to become a leader).
+ if (serviceFuture == null) {
+ return;
+ }
- if (serviceFuture != null) {
- serviceFuture.thenCompose(service ->
service.isCurrentNodeLeader().thenCompose(isLeader -> {
- if (!isLeader) {
- return nullCompletedFuture();
- }
-
- return service.updateLearners(term);
- }));
+ synchronized (raftServiceLock) {
+ if (topologyReconfigurationFuture == null) {
+ topologyReconfigurationFuture =
+ serviceFuture.thenCompose(service ->
updateLearnersOnLeader(service, term));
+ } else {
+ topologyReconfigurationFuture =
+ // At the moment topology reconfiguration will stop if
the previous one fails (see how thenCompose works).
+ // This is going to change in the future when cmg/mg
majority loss is handled properly.
+ topologyReconfigurationFuture.thenCompose(v ->
+ serviceFuture.thenCompose(service ->
updateLearnersOnLeader(service, term))
+ );
+ }
}
}
+ private static CompletableFuture<Void>
updateLearnersOnLeader(CmgRaftService service, long term) {
Review Comment:
I'd rather name it maybeUpdateLearnersOnLeader since in case of !IsLeader we
won't do an update.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -957,18 +960,39 @@ private void onLogicalTopologyChanged(long term) {
// If the future is not here yet, this means we are still starting, so
learners will be updated after start
// (if we happen to become a leader).
+ if (serviceFuture == null) {
+ return;
+ }
- if (serviceFuture != null) {
- serviceFuture.thenCompose(service ->
service.isCurrentNodeLeader().thenCompose(isLeader -> {
- if (!isLeader) {
- return nullCompletedFuture();
- }
-
- return service.updateLearners(term);
- }));
+ synchronized (raftServiceLock) {
+ if (topologyReconfigurationFuture == null) {
+ topologyReconfigurationFuture =
+ serviceFuture.thenCompose(service ->
updateLearnersOnLeader(service, term));
+ } else {
+ topologyReconfigurationFuture =
+ // At the moment topology reconfiguration will stop if
the previous one fails (see how thenCompose works).
+ // This is going to change in the future when cmg/mg
majority loss is handled properly.
+ topologyReconfigurationFuture.thenCompose(v ->
+ serviceFuture.thenCompose(service ->
updateLearnersOnLeader(service, term))
Review Comment:
busyLocks are likely missing in theCompose clauses.
##########
modules/runner/src/testFixtures/java/org/apache/ignite/internal/Cluster.java:
##########
@@ -448,7 +447,7 @@ public Ignite startNode(int index) {
*/
public Ignite startNode(int index, String nodeBootstrapConfigTemplate) {
ServerRegistration registration = startEmbeddedNode(index,
nodeBootstrapConfigTemplate);
- assertThat("nodeIndex=" + index, registration.registrationFuture(),
willSucceedIn(20, SECONDS));
+ assertThat("nodeIndex=" + index, registration.registrationFuture(),
willCompleteSuccessfully());
Review Comment:
As far as I remember willCompleteSuccessfully waits for 10 seconds. If it's
true, why it's now enough to wait 10 instead of 20 seconds?
--
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
