You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sashapolo (via GitHub)" <gi...@apache.org> on 2024/10/11 08:58:35 UTC
[PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
sashapolo opened a new pull request, #4543:
URL: https://github.com/apache/ignite-3/pull/4543
https://issues.apache.org/jira/browse/IGNITE-23404
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1802667841
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.raft;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Represents a context containing data for {@code RaftGroupServiceImpl#sendWithRetry} methods.
+ *
+ * <p>Not thread-safe.
+ */
+class RetryContext {
Review Comment:
It's not thread safe because it's expected to be used in ScheduledExecutorService only? And executor is expected to be single threaded, right? Even if it's true - it's rather fragile. I'd rather add several volatiles to traceId etc.
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo merged PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1802689798
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.raft;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Represents a context containing data for {@code RaftGroupServiceImpl#sendWithRetry} methods.
+ *
+ * <p>Not thread-safe.
+ */
+class RetryContext {
Review Comment:
> It's not thread safe because it's expected to be used in ScheduledExecutorService only? And executor is expected to be single threaded, right?
no, it is only expected to be used in a single chain of retries. It effectively means that there is no concurrent access here, we just pass this context through consecutive `submit` calls (which implies happens-before).
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.raft;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Represents a context containing data for {@code RaftGroupServiceImpl#sendWithRetry} methods.
+ *
+ * <p>Not thread-safe.
+ */
+class RetryContext {
Review Comment:
> It's not thread safe because it's expected to be used in ScheduledExecutorService only? And executor is expected to be single threaded, right?
no, it is only expected to be used in a single chain of retries. It effectively means that there is no concurrent access here, we just pass this context through consecutive `submit` calls (which implies the happens-before relationship).
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1797215933
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/RaftGroupServiceTest.java:
##########
@@ -623,6 +628,28 @@ public void testReadIndexWithMessageSendTimeout() {
assertThat(fut, willThrowFast(TimeoutException.class));
}
+ @ParameterizedTest
+ @EnumSource(names = {"ESHUTDOWN", "EHOSTDOWN", "ENODESHUTDOWN", "EPERM", "UNKNOWN", "EINTERNAL"})
+ public void testRetryOnErrorWithUpdateLeader(RaftError error) {
Review Comment:
I had this test previously, when we handled this situation in a special way. Will bring it back
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1797216914
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -687,36 +690,50 @@ private void handleErrorResponse(
case EBUSY:
case EAGAIN:
- scheduleRetry(() -> sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1));
+ scheduleRetry(peer, requestFactory, stopTime, fut, retryCount);
break;
- case ENOENT:
- scheduleRetry(() -> {
- // If changing peers or requesting a leader and something is not found
- // probably target peer is doing rebalancing, try another peer.
- if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
- sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1);
- } else {
- sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1);
- }
- });
+ case ENOENT: {
+ Peer newTargetPeer;
+
+ // If changing peers or requesting a leader and something is not found
+ // probably target peer is doing rebalancing, try another peer.
+ if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
+ newTargetPeer = randomNode(peer);
+ } else {
+ newTargetPeer = peer;
+ }
+
+ scheduleRetry(newTargetPeer, requestFactory, stopTime, fut, retryCount);
break;
+ }
+ case EHOSTDOWN:
+ case ESHUTDOWN:
+ case ENODESHUTDOWN:
case EPERM:
// TODO: IGNITE-15706
case UNKNOWN:
- case EINTERNAL:
+ case EINTERNAL: {
+ Peer newTargetPeer;
+
if (resp.leaderId() == null) {
- scheduleRetry(() -> sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1));
+ newTargetPeer = randomNode(peer);
Review Comment:
> Basically we already have a retryContext: stopTime, fut, retryCount. I'd literally incapsulate that into new RetryContext class along with adding stoppedPeers set (or excludeFromRetiresPeers, naming might be different).
That's exactly what I was trying to do, and it results in a quite a lot of changes and more complicated code. If we are ok with this, I'll do it. But how useful is this change? Shutdown nodes will be eventually removed from the topology and from the `randomNode` results, because it has a topology filter inside.
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1797218131
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -608,16 +607,20 @@ private <R extends NetworkMessage> void sendWithRetry(
err == null ? null : err.getMessage());
}
- if (err != null) {
- handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof ErrorResponse) {
- handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof SMErrorResponse) {
- handleSmErrorResponse((SMErrorResponse) resp, fut);
- } else {
- leader = peer; // The OK response was received from a leader.
-
- fut.complete((R) resp);
+ try {
+ if (err != null) {
+ handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
+ } else if (resp instanceof ErrorResponse) {
+ handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
+ } else if (resp instanceof SMErrorResponse) {
+ handleSmErrorResponse((SMErrorResponse) resp, fut);
+ } else {
+ leader = peer; // The OK response was received from a leader.
+
+ fut.complete((R) resp);
+ }
+ } catch (Throwable e) {
Review Comment:
It's a remnant of one of the previous approaches. Maybe it's not needed now, but I would prefer to leave it, because currently any of the exceptions that happen *inside* the error handling code will be ignored
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1802689798
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.raft;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Represents a context containing data for {@code RaftGroupServiceImpl#sendWithRetry} methods.
+ *
+ * <p>Not thread-safe.
+ */
+class RetryContext {
Review Comment:
> It's not thread safe because it's expected to be used in ScheduledExecutorService only? And executor is expected to be single threaded, right?
no, it is only expected to be used in a single chain of retries. It effectively means that there is no concurrent access here, we just pass this context through consecutive `submit` calls.
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1802710667
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.raft;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Represents a context containing data for {@code RaftGroupServiceImpl#sendWithRetry} methods.
+ *
+ * <p>Not thread-safe.
+ */
+class RetryContext {
Review Comment:
How do you guarantee e.g. contextTraceId visibility? handle<> might be called within different threads, thus traceId initialisation might not be visible by another thread. Where am I wrong?
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1799775627
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -556,167 +557,172 @@ private <R extends NetworkMessage> CompletableFuture<R> sendWithRetry(
) {
var future = new CompletableFuture<R>();
- sendWithRetry(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), future, 1);
+ var context = new RetryContext(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), 0);
+
+ sendWithRetry(future, context);
return future;
}
/**
* Retries a request until success or timeout.
*
- * @param peer Initial target peer, request can be sent to a random peer if the target peer is unavailable.
- * @param requestFactory Factory for creating requests to the target peer.
- * @param stopTime Stop time.
- * @param fut The future.
+ * @param fut Result future.
+ * @param retryContext Context.
* @param <R> Response type.
- * @param retryCount Number of retries made. sendWithRetry method has a recursion nature, in case of recoverable exceptions or peer
- * unavailability it'll be scheduled for a next attempt. Generally a request will be retried until success or timeout.
*/
- private <R extends NetworkMessage> void sendWithRetry(
- Peer peer,
- Function<Peer, ? extends NetworkMessage> requestFactory,
- long stopTime,
- CompletableFuture<R> fut,
- int retryCount
-
- ) {
+ private <R extends NetworkMessage> void sendWithRetry(CompletableFuture<R> fut, RetryContext retryContext) {
if (!busyLock.enterBusy()) {
fut.cancel(true);
return;
}
try {
- if (currentTimeMillis() >= stopTime) {
- fut.completeExceptionally(
- new TimeoutException(format("Send with retry timed out [retryCount = {}, groupId = {}].", retryCount, groupId)));
+ if (currentTimeMillis() >= retryContext.stopTime()) {
+ fut.completeExceptionally(new TimeoutException(format(
+ "Send with retry timed out [retryCount = {}, groupId = {}].",
+ retryContext.retryCount(),
+ groupId
+ )));
return;
}
- NetworkMessage request = requestFactory.apply(peer);
+ NetworkMessage request = retryContext.request();
- resolvePeer(peer)
+ resolvePeer(retryContext.targetPeer())
.thenCompose(node -> cluster.messagingService().invoke(node, request, configuration.responseTimeout().value()))
.whenComplete((resp, err) -> {
if (LOG.isTraceEnabled()) {
LOG.trace("sendWithRetry req={} resp={} from={} to={} err={}",
request,
resp,
cluster.topologyService().localMember().address(),
- peer.consistentId(),
+ retryContext.targetPeer().consistentId(),
err == null ? null : err.getMessage());
}
- if (err != null) {
- handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof ErrorResponse) {
- handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof SMErrorResponse) {
- handleSmErrorResponse((SMErrorResponse) resp, fut);
- } else {
- leader = peer; // The OK response was received from a leader.
-
- fut.complete((R) resp);
+ try {
+ if (err != null) {
+ handleThrowable(fut, err, retryContext);
+ } else if (resp instanceof ErrorResponse) {
+ handleErrorResponse(fut, (ErrorResponse) resp, retryContext);
+ } else if (resp instanceof SMErrorResponse) {
+ handleSmErrorResponse(fut, (SMErrorResponse) resp);
+ } else {
+ leader = retryContext.targetPeer(); // The OK response was received from a leader.
+
+ fut.complete((R) resp);
+ }
+ } catch (Throwable e) {
+ fut.completeExceptionally(e);
Review Comment:
What will happen if there's only only peer in the list and it's shutting down (returns ENODESHUTDOWN)? Are we going to retry the request?
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1802689798
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.raft;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Represents a context containing data for {@code RaftGroupServiceImpl#sendWithRetry} methods.
+ *
+ * <p>Not thread-safe.
+ */
+class RetryContext {
Review Comment:
> It's not thread safe because it's expected to be used in ScheduledExecutorService only? And executor is expected to be single threaded, right?
no, it is only expected to be used in a single chain of retries. It effectively means that there is no concurrent access here, we just pass this context through consecutive `Executor.submit` calls (which implies the happens-before relationship).
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1802734080
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RetryContext.java:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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.raft;
+
+import java.util.HashSet;
+import java.util.Set;
+import java.util.UUID;
+import java.util.function.Function;
+import org.apache.ignite.internal.network.NetworkMessage;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Represents a context containing data for {@code RaftGroupServiceImpl#sendWithRetry} methods.
+ *
+ * <p>Not thread-safe.
+ */
+class RetryContext {
Review Comment:
As you wrote
> Memory consistency effects: Actions in a thread prior to submitting a Runnable object to an Executor happen-before its execution begins, perhaps in another thread
so, there's probably no race.
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1797170114
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -687,36 +690,50 @@ private void handleErrorResponse(
case EBUSY:
case EAGAIN:
- scheduleRetry(() -> sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1));
+ scheduleRetry(peer, requestFactory, stopTime, fut, retryCount);
break;
- case ENOENT:
- scheduleRetry(() -> {
- // If changing peers or requesting a leader and something is not found
- // probably target peer is doing rebalancing, try another peer.
- if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
- sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1);
- } else {
- sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1);
- }
- });
+ case ENOENT: {
+ Peer newTargetPeer;
+
+ // If changing peers or requesting a leader and something is not found
+ // probably target peer is doing rebalancing, try another peer.
+ if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
+ newTargetPeer = randomNode(peer);
+ } else {
+ newTargetPeer = peer;
+ }
+
+ scheduleRetry(newTargetPeer, requestFactory, stopTime, fut, retryCount);
break;
+ }
+ case EHOSTDOWN:
+ case ESHUTDOWN:
+ case ENODESHUTDOWN:
case EPERM:
// TODO: IGNITE-15706
case UNKNOWN:
- case EINTERNAL:
+ case EINTERNAL: {
+ Peer newTargetPeer;
+
if (resp.leaderId() == null) {
- scheduleRetry(() -> sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1));
+ newTargetPeer = randomNode(peer);
Review Comment:
Generally that doesn't look right. First of all, because of the fact that we will hide the fact that all nodes were down with TimeoutException. Besides that it's rather not a good idea to retry the request on same stopped node. Yep, formally it's possible that it'll be restarted soon enough, but in real life we will barely face it.
Basically we already have a retryContext: stopTime, fut, retryCount. I'd literally incapsulate that into new RetryContext class along with adding stoppedPeers set (or excludeFromRetiresPeers, naming might be different). If all possible nodes are stopped - complete the future with new corresponding exception. Of course that also means that it's required to handle it properly. Initially handling might be the same as TimeoutException one with TODO's for further enhancements.
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -608,16 +607,20 @@ private <R extends NetworkMessage> void sendWithRetry(
err == null ? null : err.getMessage());
}
- if (err != null) {
- handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof ErrorResponse) {
- handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof SMErrorResponse) {
- handleSmErrorResponse((SMErrorResponse) resp, fut);
- } else {
- leader = peer; // The OK response was received from a leader.
-
- fut.complete((R) resp);
+ try {
+ if (err != null) {
+ handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
+ } else if (resp instanceof ErrorResponse) {
+ handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
+ } else if (resp instanceof SMErrorResponse) {
+ handleSmErrorResponse((SMErrorResponse) resp, fut);
+ } else {
+ leader = peer; // The OK response was received from a leader.
+
+ fut.complete((R) resp);
+ }
+ } catch (Throwable e) {
Review Comment:
What kind of exceptions do you expect here?
Seems that we already have (exceptions (!errorResponses)) handling logic incapsulated inside
```
if (err != null) {
handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
}
```
##########
modules/raft/src/test/java/org/apache/ignite/internal/raft/RaftGroupServiceTest.java:
##########
@@ -623,6 +628,28 @@ public void testReadIndexWithMessageSendTimeout() {
assertThat(fut, willThrowFast(TimeoutException.class));
}
+ @ParameterizedTest
+ @EnumSource(names = {"ESHUTDOWN", "EHOSTDOWN", "ENODESHUTDOWN", "EPERM", "UNKNOWN", "EINTERNAL"})
+ public void testRetryOnErrorWithUpdateLeader(RaftError error) {
Review Comment:
Seems that test that asserts expected exception in case of all down nodes is missing.
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1799835792
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -556,167 +557,172 @@ private <R extends NetworkMessage> CompletableFuture<R> sendWithRetry(
) {
var future = new CompletableFuture<R>();
- sendWithRetry(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), future, 1);
+ var context = new RetryContext(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), 0);
+
+ sendWithRetry(future, context);
return future;
}
/**
* Retries a request until success or timeout.
*
- * @param peer Initial target peer, request can be sent to a random peer if the target peer is unavailable.
- * @param requestFactory Factory for creating requests to the target peer.
- * @param stopTime Stop time.
- * @param fut The future.
+ * @param fut Result future.
+ * @param retryContext Context.
* @param <R> Response type.
- * @param retryCount Number of retries made. sendWithRetry method has a recursion nature, in case of recoverable exceptions or peer
- * unavailability it'll be scheduled for a next attempt. Generally a request will be retried until success or timeout.
*/
- private <R extends NetworkMessage> void sendWithRetry(
- Peer peer,
- Function<Peer, ? extends NetworkMessage> requestFactory,
- long stopTime,
- CompletableFuture<R> fut,
- int retryCount
-
- ) {
+ private <R extends NetworkMessage> void sendWithRetry(CompletableFuture<R> fut, RetryContext retryContext) {
if (!busyLock.enterBusy()) {
fut.cancel(true);
return;
}
try {
- if (currentTimeMillis() >= stopTime) {
- fut.completeExceptionally(
- new TimeoutException(format("Send with retry timed out [retryCount = {}, groupId = {}].", retryCount, groupId)));
+ if (currentTimeMillis() >= retryContext.stopTime()) {
+ fut.completeExceptionally(new TimeoutException(format(
+ "Send with retry timed out [retryCount = {}, groupId = {}].",
+ retryContext.retryCount(),
+ groupId
+ )));
return;
}
- NetworkMessage request = requestFactory.apply(peer);
+ NetworkMessage request = retryContext.request();
- resolvePeer(peer)
+ resolvePeer(retryContext.targetPeer())
.thenCompose(node -> cluster.messagingService().invoke(node, request, configuration.responseTimeout().value()))
.whenComplete((resp, err) -> {
if (LOG.isTraceEnabled()) {
LOG.trace("sendWithRetry req={} resp={} from={} to={} err={}",
request,
resp,
cluster.topologyService().localMember().address(),
- peer.consistentId(),
+ retryContext.targetPeer().consistentId(),
err == null ? null : err.getMessage());
}
- if (err != null) {
- handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof ErrorResponse) {
- handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof SMErrorResponse) {
- handleSmErrorResponse((SMErrorResponse) resp, fut);
- } else {
- leader = peer; // The OK response was received from a leader.
-
- fut.complete((R) resp);
+ try {
+ if (err != null) {
+ handleThrowable(fut, err, retryContext);
+ } else if (resp instanceof ErrorResponse) {
+ handleErrorResponse(fut, (ErrorResponse) resp, retryContext);
+ } else if (resp instanceof SMErrorResponse) {
+ handleSmErrorResponse(fut, (SMErrorResponse) resp);
+ } else {
+ leader = retryContext.targetPeer(); // The OK response was received from a leader.
+
+ fut.complete((R) resp);
+ }
+ } catch (Throwable e) {
+ fut.completeExceptionally(e);
Review Comment:
Well, following will contradicts with what I previously wrote. Sorry for that. For now I believe that we should:
- Retry the operation until timeout.
- Exclude dead nodes.
- If all nodes are touched - clean up excluded nodes and retry.
- On success - clean up excluded nodes.
Example 1:
0. peers(A,B,C)
1. touch A -> ENODESHUTDOWN -> excluded(A)
2. touch random(B,C) -> B -> ENODESHUTDOWN -> excluded(B)
3. touch random(C) -> C -> ENODESHUTDOWN -> excluded(C) -> excluded.cleanup()
4. touch random(A,B,C)
5. ...
Example 2:
0. peers(A,B,C)
1. touch A -> ENODESHUTDOWN -> excluded(A)
2. touch random(B,C) -> B -> leader -> excluded.cleanup()
Why it's important?
It's quite common to update peers on raft rebalance. E.g. raft was moved from (A,B,C) to (D,E,F)
Raft client on A wasn't updated yet, and thus knows only about A peer. Instead of failing fast on A initial touch we should wait until timeout for raft client update. Within update we will see new peers (D,E,F) and will try to send request there.
An open question is how to propagate failure context into TimeoutException. Probably we may extend TimeoutMessage with peers -> response mapping. Not sure.
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1797216914
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -687,36 +690,50 @@ private void handleErrorResponse(
case EBUSY:
case EAGAIN:
- scheduleRetry(() -> sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1));
+ scheduleRetry(peer, requestFactory, stopTime, fut, retryCount);
break;
- case ENOENT:
- scheduleRetry(() -> {
- // If changing peers or requesting a leader and something is not found
- // probably target peer is doing rebalancing, try another peer.
- if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
- sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1);
- } else {
- sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1);
- }
- });
+ case ENOENT: {
+ Peer newTargetPeer;
+
+ // If changing peers or requesting a leader and something is not found
+ // probably target peer is doing rebalancing, try another peer.
+ if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
+ newTargetPeer = randomNode(peer);
+ } else {
+ newTargetPeer = peer;
+ }
+
+ scheduleRetry(newTargetPeer, requestFactory, stopTime, fut, retryCount);
break;
+ }
+ case EHOSTDOWN:
+ case ESHUTDOWN:
+ case ENODESHUTDOWN:
case EPERM:
// TODO: IGNITE-15706
case UNKNOWN:
- case EINTERNAL:
+ case EINTERNAL: {
+ Peer newTargetPeer;
+
if (resp.leaderId() == null) {
- scheduleRetry(() -> sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1));
+ newTargetPeer = randomNode(peer);
Review Comment:
> Basically we already have a retryContext: stopTime, fut, retryCount. I'd literally incapsulate that into new RetryContext class along with adding stoppedPeers set (or excludeFromRetiresPeers, naming might be different).
That's exactly what I was trying to do, and it results in a quite a lot of changes and more complicated code. If we are ok with this, I'll do it. But how useful is this change? Shutdown nodes will be eventually removed from the topology and from the `randomNode` results, because it has a topology filter inside.
The other thing is: `peers` is a `volatile` field and it may get updated externally, which actually happens during MS or CMG repair. So we will also need a way to update all ongoing `RetryContext`s. Again, doable, but that requires adding tracking code, which complicates the solution even more
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1799835792
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -556,167 +557,172 @@ private <R extends NetworkMessage> CompletableFuture<R> sendWithRetry(
) {
var future = new CompletableFuture<R>();
- sendWithRetry(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), future, 1);
+ var context = new RetryContext(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), 0);
+
+ sendWithRetry(future, context);
return future;
}
/**
* Retries a request until success or timeout.
*
- * @param peer Initial target peer, request can be sent to a random peer if the target peer is unavailable.
- * @param requestFactory Factory for creating requests to the target peer.
- * @param stopTime Stop time.
- * @param fut The future.
+ * @param fut Result future.
+ * @param retryContext Context.
* @param <R> Response type.
- * @param retryCount Number of retries made. sendWithRetry method has a recursion nature, in case of recoverable exceptions or peer
- * unavailability it'll be scheduled for a next attempt. Generally a request will be retried until success or timeout.
*/
- private <R extends NetworkMessage> void sendWithRetry(
- Peer peer,
- Function<Peer, ? extends NetworkMessage> requestFactory,
- long stopTime,
- CompletableFuture<R> fut,
- int retryCount
-
- ) {
+ private <R extends NetworkMessage> void sendWithRetry(CompletableFuture<R> fut, RetryContext retryContext) {
if (!busyLock.enterBusy()) {
fut.cancel(true);
return;
}
try {
- if (currentTimeMillis() >= stopTime) {
- fut.completeExceptionally(
- new TimeoutException(format("Send with retry timed out [retryCount = {}, groupId = {}].", retryCount, groupId)));
+ if (currentTimeMillis() >= retryContext.stopTime()) {
+ fut.completeExceptionally(new TimeoutException(format(
+ "Send with retry timed out [retryCount = {}, groupId = {}].",
+ retryContext.retryCount(),
+ groupId
+ )));
return;
}
- NetworkMessage request = requestFactory.apply(peer);
+ NetworkMessage request = retryContext.request();
- resolvePeer(peer)
+ resolvePeer(retryContext.targetPeer())
.thenCompose(node -> cluster.messagingService().invoke(node, request, configuration.responseTimeout().value()))
.whenComplete((resp, err) -> {
if (LOG.isTraceEnabled()) {
LOG.trace("sendWithRetry req={} resp={} from={} to={} err={}",
request,
resp,
cluster.topologyService().localMember().address(),
- peer.consistentId(),
+ retryContext.targetPeer().consistentId(),
err == null ? null : err.getMessage());
}
- if (err != null) {
- handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof ErrorResponse) {
- handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof SMErrorResponse) {
- handleSmErrorResponse((SMErrorResponse) resp, fut);
- } else {
- leader = peer; // The OK response was received from a leader.
-
- fut.complete((R) resp);
+ try {
+ if (err != null) {
+ handleThrowable(fut, err, retryContext);
+ } else if (resp instanceof ErrorResponse) {
+ handleErrorResponse(fut, (ErrorResponse) resp, retryContext);
+ } else if (resp instanceof SMErrorResponse) {
+ handleSmErrorResponse(fut, (SMErrorResponse) resp);
+ } else {
+ leader = retryContext.targetPeer(); // The OK response was received from a leader.
+
+ fut.complete((R) resp);
+ }
+ } catch (Throwable e) {
+ fut.completeExceptionally(e);
Review Comment:
Well, following will contradict with what I previously wrote. Sorry for that. For now I believe that we should:
- Retry the operation until timeout.
- Exclude dead nodes.
- If all nodes are touched - clean up excluded nodes and retry.
- On success - clean up excluded nodes.
Example 1:
0. peers(A,B,C)
1. touch A -> ENODESHUTDOWN -> excluded(A)
2. touch random(B,C) -> B -> ENODESHUTDOWN -> excluded(B)
3. touch random(C) -> C -> ENODESHUTDOWN -> excluded(C) -> excluded.cleanup()
4. touch random(A,B,C)
5. ...
Example 2:
0. peers(A,B,C)
1. touch A -> ENODESHUTDOWN -> excluded(A)
2. touch random(B,C) -> B -> leader -> excluded.cleanup()
Why it's important?
It's quite common to update peers on raft rebalance. E.g. raft was moved from (A,B,C) to (D,E,F)
Raft client on A wasn't updated yet, and thus knows only about A peer. Instead of failing fast on A initial touch we should wait until timeout for raft client update. Within update we will see new peers (D,E,F) and will try to send request there.
An open question is how to propagate failure context into TimeoutException. Probably we may extend TimeoutMessage with peers -> response mapping. Not sure.
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1797216914
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -687,36 +690,50 @@ private void handleErrorResponse(
case EBUSY:
case EAGAIN:
- scheduleRetry(() -> sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1));
+ scheduleRetry(peer, requestFactory, stopTime, fut, retryCount);
break;
- case ENOENT:
- scheduleRetry(() -> {
- // If changing peers or requesting a leader and something is not found
- // probably target peer is doing rebalancing, try another peer.
- if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
- sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1);
- } else {
- sendWithRetry(peer, requestFactory, stopTime, fut, retryCount + 1);
- }
- });
+ case ENOENT: {
+ Peer newTargetPeer;
+
+ // If changing peers or requesting a leader and something is not found
+ // probably target peer is doing rebalancing, try another peer.
+ if (sentRequest instanceof GetLeaderRequest || sentRequest instanceof ChangePeersAndLearnersAsyncRequest) {
+ newTargetPeer = randomNode(peer);
+ } else {
+ newTargetPeer = peer;
+ }
+
+ scheduleRetry(newTargetPeer, requestFactory, stopTime, fut, retryCount);
break;
+ }
+ case EHOSTDOWN:
+ case ESHUTDOWN:
+ case ENODESHUTDOWN:
case EPERM:
// TODO: IGNITE-15706
case UNKNOWN:
- case EINTERNAL:
+ case EINTERNAL: {
+ Peer newTargetPeer;
+
if (resp.leaderId() == null) {
- scheduleRetry(() -> sendWithRetry(randomNode(peer), requestFactory, stopTime, fut, retryCount + 1));
+ newTargetPeer = randomNode(peer);
Review Comment:
> Basically we already have a retryContext: stopTime, fut, retryCount. I'd literally incapsulate that into new RetryContext class along with adding stoppedPeers set (or excludeFromRetiresPeers, naming might be different).
That's exactly what I was trying to do, and it results in a quite a lot of changes and more complicated code. If we are ok with this, I'll do it. But how useful is this change? Shutdown nodes will be eventually removed from the topology and from the `randomNode` results, because it has a topology filter inside. Anyway, I personally like the `RetryContext` approach, I just thought that it is quite a big change for a not obvious benefit
--
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
Re: [PR] IGNITE-23404 Properly handle EHOSTDOWN error in RaftGroupServiceImpl [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #4543:
URL: https://github.com/apache/ignite-3/pull/4543#discussion_r1799785244
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/RaftGroupServiceImpl.java:
##########
@@ -556,167 +557,172 @@ private <R extends NetworkMessage> CompletableFuture<R> sendWithRetry(
) {
var future = new CompletableFuture<R>();
- sendWithRetry(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), future, 1);
+ var context = new RetryContext(peer, requestFactory, currentTimeMillis() + configuration.retryTimeout().value(), 0);
+
+ sendWithRetry(future, context);
return future;
}
/**
* Retries a request until success or timeout.
*
- * @param peer Initial target peer, request can be sent to a random peer if the target peer is unavailable.
- * @param requestFactory Factory for creating requests to the target peer.
- * @param stopTime Stop time.
- * @param fut The future.
+ * @param fut Result future.
+ * @param retryContext Context.
* @param <R> Response type.
- * @param retryCount Number of retries made. sendWithRetry method has a recursion nature, in case of recoverable exceptions or peer
- * unavailability it'll be scheduled for a next attempt. Generally a request will be retried until success or timeout.
*/
- private <R extends NetworkMessage> void sendWithRetry(
- Peer peer,
- Function<Peer, ? extends NetworkMessage> requestFactory,
- long stopTime,
- CompletableFuture<R> fut,
- int retryCount
-
- ) {
+ private <R extends NetworkMessage> void sendWithRetry(CompletableFuture<R> fut, RetryContext retryContext) {
if (!busyLock.enterBusy()) {
fut.cancel(true);
return;
}
try {
- if (currentTimeMillis() >= stopTime) {
- fut.completeExceptionally(
- new TimeoutException(format("Send with retry timed out [retryCount = {}, groupId = {}].", retryCount, groupId)));
+ if (currentTimeMillis() >= retryContext.stopTime()) {
+ fut.completeExceptionally(new TimeoutException(format(
+ "Send with retry timed out [retryCount = {}, groupId = {}].",
+ retryContext.retryCount(),
+ groupId
+ )));
return;
}
- NetworkMessage request = requestFactory.apply(peer);
+ NetworkMessage request = retryContext.request();
- resolvePeer(peer)
+ resolvePeer(retryContext.targetPeer())
.thenCompose(node -> cluster.messagingService().invoke(node, request, configuration.responseTimeout().value()))
.whenComplete((resp, err) -> {
if (LOG.isTraceEnabled()) {
LOG.trace("sendWithRetry req={} resp={} from={} to={} err={}",
request,
resp,
cluster.topologyService().localMember().address(),
- peer.consistentId(),
+ retryContext.targetPeer().consistentId(),
err == null ? null : err.getMessage());
}
- if (err != null) {
- handleThrowable(err, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof ErrorResponse) {
- handleErrorResponse((ErrorResponse) resp, peer, request, requestFactory, stopTime, fut, retryCount);
- } else if (resp instanceof SMErrorResponse) {
- handleSmErrorResponse((SMErrorResponse) resp, fut);
- } else {
- leader = peer; // The OK response was received from a leader.
-
- fut.complete((R) resp);
+ try {
+ if (err != null) {
+ handleThrowable(fut, err, retryContext);
+ } else if (resp instanceof ErrorResponse) {
+ handleErrorResponse(fut, (ErrorResponse) resp, retryContext);
+ } else if (resp instanceof SMErrorResponse) {
+ handleSmErrorResponse(fut, (SMErrorResponse) resp);
+ } else {
+ leader = retryContext.targetPeer(); // The OK response was received from a leader.
+
+ fut.complete((R) resp);
+ }
+ } catch (Throwable e) {
+ fut.completeExceptionally(e);
Review Comment:
> Are we going to retry the request?
No, we are going to fail with an error, because there are no live peers left
--
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