You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2023/10/25 17:50:10 UTC
[PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
dcapwell opened a new pull request, #2844:
URL: https://github.com/apache/cassandra/pull/2844
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372139729
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -165,13 +207,32 @@ public void onFailure(InetAddressAndPort from, RequestFailureReason failureReaso
backoff.computeWaitTime(attempt), backoff.unit());
return;
}
+ maybeRecordRetry(failureReason);
finalCallback.onFailure(from, failureReason);
return;
default:
throw new AssertionError("Unknown error handler: " + allowed);
}
}
+ private void maybeRecordRetry(@Nullable RequestFailureReason reason)
Review Comment:
from https://issues.apache.org/jira/browse/CASSANDRA-18952
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372144972
##########
test/unit/org/apache/cassandra/repair/FuzzTestBase.java:
##########
@@ -278,6 +282,12 @@ public void setupSchema()
createSchema();
}
+ protected void cleanupRepairTables()
+ {
+ for (String table : Arrays.asList(SystemKeyspace.REPAIRS))
Review Comment:
IR keeps trying to load the system table, which can slow down the tests... so added this as a work around... it would be great if storage could be isolated and wasn't global state =(
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372148939
##########
test/unit/org/apache/cassandra/repair/FuzzTestBase.java:
##########
@@ -850,7 +867,14 @@ private <REQ, RSP> void maybeEnqueue(Message<REQ> message, InetAddressAndPort to
if (ctx != null)
{
assert ctx == cb;
- ctx.onFailure(to, RequestFailureReason.TIMEOUT);
+ try
+ {
+ ctx.onFailure(to, RequestFailureReason.TIMEOUT);
+ }
+ catch (Throwable t)
+ {
+ failures.add(t);
Review Comment:
when IR messages retry they would NPE as they use a null `desc`... this would get handled by the future and ignored, so tests would fail saying the repair didn't complete rather than the NPE... this fix makes sure we fail fast
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372133075
##########
src/java/org/apache/cassandra/repair/RepairMessageVerbHandler.java:
##########
@@ -286,27 +285,28 @@ public void doVerb(final Message<RepairMessage> message)
break;
case PREPARE_CONSISTENT_REQ:
- ctx.repair().consistent.local.handlePrepareMessage(message.from(), (PrepareConsistentRequest) message.payload);
+ ctx.repair().consistent.local.handlePrepareMessage(message);
break;
case PREPARE_CONSISTENT_RSP:
- ctx.repair().consistent.coordinated.handlePrepareResponse((PrepareConsistentResponse) message.payload);
+ ctx.repair().consistent.coordinated.handlePrepareResponse(message);
break;
case FINALIZE_PROPOSE_MSG:
- ctx.repair().consistent.local.handleFinalizeProposeMessage(message.from(), (FinalizePropose) message.payload);
+ ctx.repair().consistent.local.handleFinalizeProposeMessage(message);
break;
case FINALIZE_PROMISE_MSG:
- ctx.repair().consistent.coordinated.handleFinalizePromiseMessage((FinalizePromise) message.payload);
+ ctx.repair().consistent.coordinated.handleFinalizePromiseMessage(message);
break;
case FINALIZE_COMMIT_MSG:
- ctx.repair().consistent.local.handleFinalizeCommitMessage(message.from(), (FinalizeCommit) message.payload);
+ ctx.repair().consistent.local.handleFinalizeCommitMessage(message);
break;
case FAILED_SESSION_MSG:
FailSession failure = (FailSession) message.payload;
+ sendAck(message);
Review Comment:
prob could nack if we don't know the session... ack vs nack really doesn't matter for IR as it doesn't wait for the result... the retries really are just for stability and not for the senders 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372138274
##########
src/java/org/apache/cassandra/repair/consistent/LocalSessions.java:
##########
@@ -637,10 +642,13 @@ public LocalSession getSession(TimeUUID sessionID)
}
@VisibleForTesting
- synchronized void putSessionUnsafe(LocalSession session)
+ synchronized boolean putSessionUnsafe(LocalSession session)
{
+ if (sessions.containsKey(session.sessionID))
Review Comment:
this actually makes it safe... but does have a big change in semantics... for retry logic we need ability to dedup, so blind overriding can't be done... I could always rename this to `putSession`....
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372123829
##########
src/java/org/apache/cassandra/repair/SharedContext.java:
##########
@@ -59,6 +59,16 @@ public interface SharedContext
ScheduledExecutorPlus optionalTasks();
MessageDelivery messaging();
+ default SharedContext withMessaging(MessageDelivery messaging)
Review Comment:
This is from https://issues.apache.org/jira/browse/CASSANDRA-18952 and no changes are from this PR... can ignore this file
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372139477
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -67,11 +86,36 @@ private enum ErrorHandling { NONE, TIMEOUT, RETRY }
map.put(Verb.SYNC_REQ, timeoutVersion);
map.put(Verb.VALIDATION_RSP, SUPPORTS_RETRY);
map.put(Verb.SYNC_RSP, SUPPORTS_RETRY);
+ // IR messages
+ map.put(Verb.PREPARE_CONSISTENT_REQ, SUPPORTS_RETRY);
+ map.put(Verb.PREPARE_CONSISTENT_RSP, SUPPORTS_RETRY);
+ map.put(Verb.FINALIZE_PROPOSE_MSG, SUPPORTS_RETRY);
+ map.put(Verb.FINALIZE_PROMISE_MSG, SUPPORTS_RETRY);
+ map.put(Verb.FINALIZE_COMMIT_MSG, SUPPORTS_RETRY);
+ map.put(Verb.FAILED_SESSION_MSG, SUPPORTS_RETRY);
VERB_TIMEOUT_VERSIONS = Collections.unmodifiableMap(map);
+
+ EnumSet<Verb> allowsRetry = EnumSet.noneOf(Verb.class);
+ allowsRetry.add(Verb.PREPARE_MSG);
+ allowsRetry.add(Verb.VALIDATION_REQ);
+ allowsRetry.add(Verb.VALIDATION_RSP);
+ allowsRetry.add(Verb.SYNC_REQ);
+ allowsRetry.add(Verb.SYNC_RSP);
+ allowsRetry.add(Verb.SNAPSHOT_MSG);
+ allowsRetry.add(Verb.CLEANUP_MSG);
+ // IR messages
+ allowsRetry.add(Verb.PREPARE_CONSISTENT_REQ);
+ allowsRetry.add(Verb.PREPARE_CONSISTENT_RSP);
+ allowsRetry.add(Verb.FINALIZE_PROPOSE_MSG);
+ allowsRetry.add(Verb.FINALIZE_PROMISE_MSG);
+ allowsRetry.add(Verb.FINALIZE_COMMIT_MSG);
+ allowsRetry.add(Verb.FAILED_SESSION_MSG);
+ ALLOWS_RETRY = Collections.unmodifiableSet(allowsRetry);
}
- private static final Set<Verb> SUPPORTS_RETRY_WITHOUT_VERSION_CHECK = Collections.unmodifiableSet(EnumSet.of(Verb.CLEANUP_MSG));
private static final Logger logger = LoggerFactory.getLogger(RepairMessage.class);
+ private static final NoSpamLogger noSpam = NoSpamLogger.getLogger(logger, 1, TimeUnit.MINUTES);
Review Comment:
from https://issues.apache.org/jira/browse/CASSANDRA-18952
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -121,27 +165,25 @@ public static <T> void sendMessageWithRetries(SharedContext ctx, RepairMessage r
public static void sendMessageWithRetries(SharedContext ctx, RepairMessage request, Verb verb, InetAddressAndPort endpoint)
{
- sendMessageWithRetries(ctx, backoff(ctx, verb), always(), request, verb, endpoint, new RequestCallback<>()
- {
- @Override
- public void onResponse(Message<Object> msg)
- {
- }
+ sendMessageWithRetries(ctx, backoff(ctx, verb), always(), request, verb, endpoint, NOOP_CALLBACK, 0);
+ }
- @Override
- public void onFailure(InetAddressAndPort from, RequestFailureReason failureReason)
- {
- }
- }, 0);
+ public static void sendMessageWithRetries(SharedContext ctx, Supplier<Boolean> allowRetry, RepairMessage request, Verb verb, InetAddressAndPort endpoint)
+ {
+ sendMessageWithRetries(ctx, backoff(ctx, verb), allowRetry, request, verb, endpoint, NOOP_CALLBACK, 0);
}
- private static <T> void sendMessageWithRetries(SharedContext ctx, Backoff backoff, Supplier<Boolean> allowRetry, RepairMessage request, Verb verb, InetAddressAndPort endpoint, RequestCallback<T> finalCallback, int attempt)
+ @VisibleForTesting
+ static <T> void sendMessageWithRetries(SharedContext ctx, Backoff backoff, Supplier<Boolean> allowRetry, RepairMessage request, Verb verb, InetAddressAndPort endpoint, RequestCallback<T> finalCallback, int attempt)
{
+ if (!ALLOWS_RETRY.contains(verb))
Review Comment:
https://issues.apache.org/jira/browse/CASSANDRA-18952
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372150280
##########
test/unit/org/apache/cassandra/repair/messages/RepairMessageTest.java:
##########
@@ -0,0 +1,182 @@
+/*
+ * 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.cassandra.repair.messages;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.cassandra.concurrent.ScheduledExecutorPlus;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.RequestFailureReason;
+import org.apache.cassandra.gms.IGossiper;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.metrics.RepairMetrics;
+import org.apache.cassandra.net.Message;
+import org.apache.cassandra.net.MessageDelivery;
+import org.apache.cassandra.net.RequestCallback;
+import org.apache.cassandra.net.Verb;
+import org.apache.cassandra.repair.SharedContext;
+import org.apache.cassandra.utils.Backoff;
+import org.apache.cassandra.utils.FBUtilities;
+import org.apache.cassandra.utils.TimeUUID;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+
+import static org.apache.cassandra.repair.messages.RepairMessage.always;
+import static org.apache.cassandra.repair.messages.RepairMessage.sendMessageWithRetries;
+import static org.apache.cassandra.test.asserts.ExtendedAssertions.assertThat;
+
+// Tests may use verb / message pairs that do not make sense... that is due to the fact that the message sending logic does not validate this and delegates such validation to messaging, which is mocked within the class...
+// By using messages with simpler state it makes the test easier to read, even though the verb -> message mapping is incorrect.
+public class RepairMessageTest
Review Comment:
this is from https://issues.apache.org/jira/browse/CASSANDRA-18952
##########
test/unit/org/apache/cassandra/test/asserts/ExtendedAssertions.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.cassandra.test.asserts;
+
+import com.codahale.metrics.Counting;
+import org.assertj.core.api.AbstractObjectAssert;
+
+public class ExtendedAssertions
Review Comment:
this is from https://issues.apache.org/jira/browse/CASSANDRA-18952
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372147987
##########
test/unit/org/apache/cassandra/repair/FuzzTestBase.java:
##########
@@ -799,6 +814,8 @@ private <REQ, RSP> void maybeEnqueue(Message<REQ> message, InetAddressAndPort to
CallbackContext cb;
if (callback != null)
{
+ if (callbacks.containsKey(message.id()))
Review Comment:
this is to better match `MessagingService`, this doesn't happen but better to be consistent
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1378032898
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -232,4 +298,15 @@ private static ErrorHandling errorHandlingSupported(SharedContext ctx, InetAddre
return ErrorHandling.TIMEOUT;
return ErrorHandling.NONE;
}
+
+ public static void sendFailureResponse(SharedContext ctx, Message<?> respondTo)
Review Comment:
> but why reply if it's just going to be dropped?
in order to know this we need to check gossip, which maybe stale. So If we think its 4.x but its actually 5.x then we avoid sending the ack/nack and it will retry...
Now, since we don't 100% support mixed version repairs, by not caring we make this simpler to reason about with the cost of empty dropped messages while in mixed mode (again, we don't support)... so this patch does not break mixed mode, but isn't 100% ideal in mixed mode...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372139274
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -67,11 +86,36 @@ private enum ErrorHandling { NONE, TIMEOUT, RETRY }
map.put(Verb.SYNC_REQ, timeoutVersion);
map.put(Verb.VALIDATION_RSP, SUPPORTS_RETRY);
map.put(Verb.SYNC_RSP, SUPPORTS_RETRY);
+ // IR messages
+ map.put(Verb.PREPARE_CONSISTENT_REQ, SUPPORTS_RETRY);
+ map.put(Verb.PREPARE_CONSISTENT_RSP, SUPPORTS_RETRY);
+ map.put(Verb.FINALIZE_PROPOSE_MSG, SUPPORTS_RETRY);
+ map.put(Verb.FINALIZE_PROMISE_MSG, SUPPORTS_RETRY);
+ map.put(Verb.FINALIZE_COMMIT_MSG, SUPPORTS_RETRY);
+ map.put(Verb.FAILED_SESSION_MSG, SUPPORTS_RETRY);
VERB_TIMEOUT_VERSIONS = Collections.unmodifiableMap(map);
+
+ EnumSet<Verb> allowsRetry = EnumSet.noneOf(Verb.class);
Review Comment:
this logic is from https://issues.apache.org/jira/browse/CASSANDRA-18952 but added IR messages as well
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1377994179
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -232,4 +298,15 @@ private static ErrorHandling errorHandlingSupported(SharedContext ctx, InetAddre
return ErrorHandling.TIMEOUT;
return ErrorHandling.NONE;
}
+
+ public static void sendFailureResponse(SharedContext ctx, Message<?> respondTo)
Review Comment:
sure, but why reply if it's just going to be dropped? It's just a cleanliness thing. That said, no one's going to notice a few empty messages getting sent into the void, so I'm fine leaving as is if you'd prefer.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #2844: CASSANDRA-18962: Add retries to IR messages
URL: https://github.com/apache/cassandra/pull/2844
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372150614
##########
test/unit/org/apache/cassandra/utils/NoSpamLoggerTest.java:
##########
@@ -80,7 +80,7 @@ public boolean equals(Object o)
@BeforeClass
public static void setUpClass() throws Exception
{
- NoSpamLogger.CLOCK = () -> now;
+ NoSpamLogger.unsafeSetClock(() -> now);
Review Comment:
this is from https://issues.apache.org/jira/browse/CASSANDRA-18952
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372141168
##########
src/java/org/apache/cassandra/utils/NoSpamLogger.java:
##########
@@ -51,19 +51,18 @@ public enum Level
}
@VisibleForTesting
- static interface Clock
+ public interface Clock
Review Comment:
this was done in https://issues.apache.org/jira/browse/CASSANDRA-18952; simulation detects that global clock is used and rejects it, so we now use the simulated clock here, which means the no spam logic happens often (simulation time moves much faster than wall clock)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372146479
##########
test/unit/org/apache/cassandra/repair/FuzzTestBase.java:
##########
@@ -319,25 +329,16 @@ static void enableMessageFaults(Cluster cluster)
@Override
public Set<Faults> apply(Cluster.Node node, Message<?> message)
{
+ if (RepairMessage.ALLOWS_RETRY.contains(message.verb()))
+ {
+ allowDrop.add(message.id());
+ return Faults.DROPPED;
+ }
switch (message.verb())
{
- case PREPARE_MSG:
- case VALIDATION_REQ:
- case VALIDATION_RSP:
- case SYNC_REQ:
- case SYNC_RSP:
- case SNAPSHOT_MSG:
- case CLEANUP_MSG:
- allowDrop.add(message.id());
- return Faults.DROPPED;
// these messages are not resilent to ephemeral issues
- case PREPARE_CONSISTENT_REQ:
- case PREPARE_CONSISTENT_RSP:
- case FINALIZE_PROPOSE_MSG:
- case FINALIZE_PROMISE_MSG:
- case FINALIZE_COMMIT_MSG:
- case FAILED_SESSION_MSG:
-
+ case STATUS_REQ:
Review Comment:
these messages actually no-op as `StorageService` is not initialized... they actually cause annoying cases as they fail sessions, so didn't try to get working... just didn't want tests to fail if those messages happen
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372144044
##########
test/unit/org/apache/cassandra/repair/FailingRepairFuzzTest.java:
##########
@@ -114,6 +114,7 @@ public void failingRepair()
}
cluster.processAll();
+ Assertions.assertThat(repair.state.isComplete()).describedAs("Repair job did not complete, and no work is pending...").isTrue();
Review Comment:
added to produce a more useful error msg when repairs are not complete... These classes all assume that nothing left to process means the repair is complete, but it could also mean a bug in repair that will never get recovered... in that case the repair is not complete and the error message is hard to understand... so this new error message is trying to express this case
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372135468
##########
src/java/org/apache/cassandra/repair/consistent/CoordinatorSession.java:
##########
@@ -154,34 +160,37 @@ synchronized boolean hasFailed()
return getState() == State.FAILED || Iterables.any(participantStates.values(), v -> v == State.FAILED);
}
- protected void sendMessage(InetAddressAndPort destination, Message<RepairMessage> message)
- {
- logger.trace("Sending {} to {}", message.payload, destination);
-
- ctx.messaging().send(message, destination);
- }
-
public Future<Void> prepare()
{
Preconditions.checkArgument(allStates(State.PREPARING));
logger.info("Beginning prepare phase of incremental repair session {}", sessionID);
- Message<RepairMessage> message =
- Message.out(Verb.PREPARE_CONSISTENT_REQ, new PrepareConsistentRequest(sessionID, coordinator, participants));
+
+ PrepareConsistentRequest request = new PrepareConsistentRequest(sessionID, coordinator, participants);
for (final InetAddressAndPort participant : participants)
{
- sendMessage(participant, message);
+ sendMessageWithRetries(ctx,
+ notDone(prepareFuture),
+ request,
+ Verb.PREPARE_CONSISTENT_REQ,
+ participant);
}
return prepareFuture;
}
- public synchronized void handlePrepareResponse(InetAddressAndPort participant, boolean success)
+ public synchronized void handlePrepareResponse(Message<PrepareConsistentResponse> msg)
{
+ InetAddressAndPort participant = msg.payload.participant;
+ boolean success = msg.payload.success;
if (getState() == State.FAILED)
{
logger.trace("Incremental repair {} has failed, ignoring prepare response from {}", sessionID, participant);
+ sendFailureResponse(ctx, msg);
return;
}
+ sendAck(ctx, msg);
+ if (getParticipantState(participant) != State.PREPARING)
Review Comment:
this also fixes a IR bug where we see the response *after* we get a fail session request... in that case we fail and log a stack trace saying we can't go from `FAILURE -> PREPARED`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372123411
##########
src/java/org/apache/cassandra/metrics/RepairMetrics.java:
##########
@@ -18,17 +18,75 @@
package org.apache.cassandra.metrics;
+import java.util.Collections;
Review Comment:
This is from https://issues.apache.org/jira/browse/CASSANDRA-18952 and no changes are from this PR... can ignore this file
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372142300
##########
test/unit/org/apache/cassandra/repair/ConcurrentIrWithPreviewFuzzTest.java:
##########
@@ -62,7 +64,7 @@ public void concurrentIrWithPreview()
// cause a delay in validation to have more failing previews
closeables.add(cluster.nodes.get(pickParticipant(rs, previewCoordinator, preview)).doValidation(next -> (cfs, validator) -> {
if (validator.desc.parentSessionId.equals(preview.state.id))
- cluster.unorderedScheduled.schedule(() -> next.accept(cfs, validator), 1, TimeUnit.HOURS);
+ delayValidation(cluster, ir, next, cfs, validator);
Review Comment:
this was a flakey issue that this patch discovered... with IR retries we can have IR take longer than 1h, so waiting by time is unsafe and leads to flakey builds... now we check IR state and keep retrying until IR completes
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372145288
##########
test/unit/org/apache/cassandra/repair/FuzzTestBase.java:
##########
@@ -319,25 +329,16 @@ static void enableMessageFaults(Cluster cluster)
@Override
public Set<Faults> apply(Cluster.Node node, Message<?> message)
{
+ if (RepairMessage.ALLOWS_RETRY.contains(message.verb()))
Review Comment:
this is from https://issues.apache.org/jira/browse/CASSANDRA-18952
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1372136297
##########
src/java/org/apache/cassandra/repair/consistent/CoordinatorSession.java:
##########
@@ -218,19 +227,29 @@ public synchronized Future<Void> finalizePropose()
{
Preconditions.checkArgument(allStates(State.REPAIRING));
logger.info("Proposing finalization of repair session {}", sessionID);
- Message<RepairMessage> message = Message.out(Verb.FINALIZE_PROPOSE_MSG, new FinalizePropose(sessionID));
+ FinalizePropose request = new FinalizePropose(sessionID);
for (final InetAddressAndPort participant : participants)
{
- sendMessage(participant, message);
+ sendMessageWithRetries(ctx, notDone(finalizeProposeFuture), request, Verb.FINALIZE_PROPOSE_MSG, participant);
}
return finalizeProposeFuture;
}
- public synchronized void handleFinalizePromise(InetAddressAndPort participant, boolean success)
+ public synchronized void handleFinalizePromise(Message<FinalizePromise> message)
{
+ InetAddressAndPort participant = message.payload.participant;
+ boolean success = message.payload.promised;
if (getState() == State.FAILED)
{
logger.trace("Incremental repair {} has failed, ignoring finalize promise from {}", sessionID, participant);
+ sendFailureResponse(ctx, message);
+ return;
+ }
+ sendAck(ctx, message);
+ if (getParticipantState(participant) != State.REPAIRING)
Review Comment:
this also fixes a IR bug where we see the response *after* we get a fail session request... in that case we fail and log a stack trace saying we can't go from `FAILURE -> FINALIZE_PROMISED`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1376731807
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -232,4 +298,15 @@ private static ErrorHandling errorHandlingSupported(SharedContext ctx, InetAddre
return ErrorHandling.TIMEOUT;
return ErrorHandling.NONE;
}
+
+ public static void sendFailureResponse(SharedContext ctx, Message<?> respondTo)
Review Comment:
I think we'll need to check the version of the node we're responding to. Otherwise I don't think they'll be expecting a response for this message
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18962: Add retries to IR messages [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #2844:
URL: https://github.com/apache/cassandra/pull/2844#discussion_r1377935929
##########
src/java/org/apache/cassandra/repair/messages/RepairMessage.java:
##########
@@ -232,4 +298,15 @@ private static ErrorHandling errorHandlingSupported(SharedContext ctx, InetAddre
return ErrorHandling.TIMEOUT;
return ErrorHandling.NONE;
}
+
+ public static void sendFailureResponse(SharedContext ctx, Message<?> respondTo)
Review Comment:
thats fine, we drop in the messaging layer... these are *replies* which require we have Callbacks define for the message number, so the below block is what we hit in 4.x
```
RequestCallbacks.CallbackInfo callbackInfo = MessagingService.instance().callbacks.remove(message.id(), message.from());
if (callbackInfo == null)
{
String msg = "Callback already removed for {} (from {})";
logger.trace(msg, message.id(), message.from());
Tracing.trace(msg, message.id(), message.from());
return;
}
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org