You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "alanwang67 (via GitHub)" <gi...@apache.org> on 2026/08/18 18:59:38 UTC
[PR] CASSANDRA-21584 Fix topology related test failures for Accord [cassandra-accord]
alanwang67 opened a new pull request, #289:
URL: https://github.com/apache/cassandra-accord/pull/289
(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-21584 Fix topology related test failures for Accord [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #289:
URL: https://github.com/apache/cassandra-accord/pull/289#discussion_r3808524222
##########
accord-core/src/test/java/accord/coordinate/TopologyChangeTest.java:
##########
@@ -185,14 +184,13 @@ void lateBarrierTest() throws Throwable
.build())
{
cluster.nodes(1, 2, 3).forEach(node -> assertEpochRejection(node, keys, 1, false));
- cluster.networkFilter.addFilter(Predicates.alwaysTrue(), to -> id(4).equals(to), TopologyChangeTest::isExclSyncPoint);
+ cluster.networkFilter.addFilter(Predicates.alwaysTrue(), to -> id(4).equals(to), TopologyChangeTest::isSyncPoint);
cluster.nodes(1, 2, 3, 4, 5).forEach(node -> node.topology().reportTopology(topology2));
cluster.nodes(4).forEach(node -> {
getUncheckedTimeout(node.topology().await(2, null), 5, TimeUnit.SECONDS);
MockTopologyService topologyService = (MockTopologyService) node.topology().topologyService();
getUncheckedTimeout(topologyService.ackFor(2).coordinate, 5, TimeUnit.SECONDS);
- assertEpochRejection(node, keys, 1, false); // shouldn't have received the sync point preaccept
Review Comment:
Speaking to Alan in slack he is saying that `Node.send` allows not sending the message to the local node and directly handle it, but when i disable that logic the test unchanged still fails the same way; so it sounds like the foundational assumption about the patch is off.
Also this assert is validating we *reject* so by removing the assert the test really isn't checking that anymore? so what is the test doing if we remove these asserts? feels out of place and the assumption that its based off doesn't seem to be true
--
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-21584 Fix topology related test failures for Accord [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #289:
URL: https://github.com/apache/cassandra-accord/pull/289#discussion_r3808531333
##########
accord-core/src/test/java/accord/coordinate/TopologyChangeTest.java:
##########
@@ -185,14 +184,13 @@ void lateBarrierTest() throws Throwable
.build())
{
cluster.nodes(1, 2, 3).forEach(node -> assertEpochRejection(node, keys, 1, false));
- cluster.networkFilter.addFilter(Predicates.alwaysTrue(), to -> id(4).equals(to), TopologyChangeTest::isExclSyncPoint);
+ cluster.networkFilter.addFilter(Predicates.alwaysTrue(), to -> id(4).equals(to), TopologyChangeTest::isSyncPoint);
cluster.nodes(1, 2, 3, 4, 5).forEach(node -> node.topology().reportTopology(topology2));
cluster.nodes(4).forEach(node -> {
getUncheckedTimeout(node.topology().await(2, null), 5, TimeUnit.SECONDS);
MockTopologyService topologyService = (MockTopologyService) node.topology().topologyService();
getUncheckedTimeout(topologyService.ackFor(2).coordinate, 5, TimeUnit.SECONDS);
- assertEpochRejection(node, keys, 1, false); // shouldn't have received the sync point preaccept
Review Comment:
here is the patch i ran the test with
```
diff --git a/accord-core/src/main/java/accord/local/Node.java b/accord-core/src/main/java/accord/local/Node.java
index 785fe9c7..1143835b 100644
--- a/accord-core/src/main/java/accord/local/Node.java
+++ b/accord-core/src/main/java/accord/local/Node.java
@@ -573,16 +573,18 @@ public class Node implements NodeCommandStoreService
public <T extends Reply> Cancellable send(Id to, Request send, @Nonnull AsyncExecutor executor, Callback<T> callback, @Nullable Tracing tracing)
{
maybeTraceRemote(to, send, tracing);
- if (permitLocalDelivery() && to.equals(id)) return new LocalDelivery<>(this, callback).deliver(send);
- else return messageSink.send(to, send, executor, callback);
+ return messageSink.send(to, send, executor, callback);
+ //if (permitLocalDelivery() && to.equals(id)) return new LocalDelivery<>(this, callback).deliver(send);
+ //else return messageSink.send(to, send, executor, callback);
}
// send to a specific node
public void send(Id to, Request send, @Nullable Tracing tracing)
{
maybeTrace(to, send, tracing);
- if (to.equals(id)) send.process(this, to, new NoReplyContext(this, send));
- else messageSink.send(to, send);
+ //if (to.equals(id)) send.process(this, to, new NoReplyContext(this, send));
+ //else messageSink.send(to, send);
+ messageSink.send(to, send);
}
private void maybeTrace(Node.Id to, Request send, @Nullable Tracing tracing)
@@ -951,4 +953,4 @@ public class Node implements NodeCommandStoreService
if (replaying) CommandsForKey.disableLinearizabilityViolationsReporting();
else CommandsForKey.enableLinearizabilityViolationsReporting();
}
-}
\ No newline at end of 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