You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "belliottsmith (via GitHub)" <gi...@apache.org> on 2024/11/20 15:07:00 UTC
[PR] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
belliottsmith opened a new pull request, #141:
URL: https://github.com/apache/cassandra-accord/pull/141
(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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852642890
##########
accord-core/src/main/java/accord/coordinate/CoordinationAdapter.java:
##########
@@ -310,14 +302,6 @@ private static class AsyncInclusiveSyncPointAdapter<U extends Unseekable> extend
protected AsyncInclusiveSyncPointAdapter() {
super();
}
-
- @Override
- protected void addOrExecuteCallback(ExecuteBlocking<U> execute, BiConsumer<? super SyncPoint<U>, Throwable> callback)
- {
- // If this is the async adapter then we want to invoke the callback immediately
Review Comment:
ok ill need to look closer, I missed that detail
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852055169
##########
accord-core/src/main/java/accord/messages/InformDurable.java:
##########
@@ -63,6 +67,22 @@ private InformDurable(TxnId txnId, Route<?> scope, long waitForEpoch, @Nullable
this.durability = durability;
}
+ public static void informHome(Node node, Topologies any, TxnId txnId, Route<?> route, Timestamp executeAt)
+ {
+ long homeEpoch = txnId.epoch();
+ Topology homeEpochTopology = any.forEpoch(homeEpoch);
+ int homeShardIndex = homeEpochTopology.indexForKey(route.homeKey());
+ if (homeShardIndex < 0)
+ {
+ homeEpochTopology = node.topology().globalForEpoch(homeEpoch);
+ homeShardIndex = homeEpochTopology.indexForKey(route.homeKey());
+ }
+
+ Shard homeShard = homeEpochTopology.get(homeShardIndex);
+ Topologies homeTopology = new Topologies.Single(any, new Topology(homeEpoch, homeShard));
+ node.send(homeShard.nodes, to -> new InformDurable(to, homeTopology, route.homeKeyOnlyRoute(), txnId, executeAt, Majority));
Review Comment:
I guess `durable` is a slightly overloaded concept. But, something is _durable_ once it is known to a majority. Basically it means can survive our tolerated number of failures. We have higher levels of durability, and confusingly we also have `Local` durability (that perhaps we should remove, since it isn't _durable_). Anyway, I'm happy to parameterise it, but `Majority` is the base level at which you can call something _durable_, so if unspecified should normally be assumed.
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith merged PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852059915
##########
accord-core/src/main/java/accord/coordinate/ExecuteSyncPoint.java:
##########
@@ -195,6 +257,11 @@ protected void onSuccess()
trySuccess(syncPoint);
}
+ protected void sendApply(Node.Id to)
Review Comment:
probably my mistake, I meant to extract and call it directly. will fix
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852048484
##########
accord-core/src/main/java/accord/utils/async/AsyncChains.java:
##########
@@ -430,8 +430,10 @@ protected Cancellable start(BiConsumer<? super V, Throwable> callback)
// either the thing we start, or the thing we do in follow-up
BiConsumer<? super V, Throwable> next;
+ private final Head<?> head;
Review Comment:
ah, I'll rebase against trunk
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852062473
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -513,10 +513,17 @@ public enum CommitOrReadNack implements ReadData.ReadReply
* The commit has been rejected due to stale ballot.
*/
Rejected("CommitRejected"),
+
/**
* Either not committed, or not stable
*/
Insufficient("CommitInsufficient"),
+
+ /**
+ * PreApplied successfully, but the request is blocking so waiting to reply
+ */
+ Waiting("ApplyWaiting"),
Review Comment:
hmm, let me see where it went - it should be here as part of the new sync point logic.
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1853915051
##########
accord-core/src/main/java/accord/coordinate/ExecuteSyncPoint.java:
##########
@@ -195,6 +257,11 @@ protected void onSuccess()
trySuccess(syncPoint);
}
+ protected void sendApply(Node.Id to)
Review Comment:
yep, forgot to invoke it instead of CoordinateSyncPoint.sendApply
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852061584
##########
accord-core/src/main/java/accord/coordinate/MaybeRecover.java:
##########
@@ -108,7 +108,7 @@ protected void onDone(Success success, Throwable fail)
if (hasMadeProgress(full))
{
if (full.durability.isDurable())
- node.send(topologies.forEpoch(txnId.epoch()).forKey(route.homeKey()).nodes, to -> new InformDurable(to, topologies, route, txnId, full.executeAtIfKnown(), full.durability));
+ InformDurable.informHome(node, topologies, txnId, route, full.executeAtIfKnown());
Review Comment:
I'll parameterise, but as mentioned in other location if unspecified the lowest level of _durable_ durability is reasonable to assume (should perhaps remove `Local` durability to make it clearer this is the lowest _durable_ durability).
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852057536
##########
accord-core/src/main/java/accord/coordinate/CoordinationAdapter.java:
##########
@@ -219,18 +224,17 @@ public void stabilise(Node node, Topologies any, FullRoute<?> route, Ballot ball
@Override
public void execute(Node node, Topologies any, FullRoute<?> route, ExecutePath path, TxnId txnId, Txn txn, Timestamp executeAt, Deps deps, BiConsumer<? super SyncPoint<U>, Throwable> callback)
{
- Topologies all = forExecution(node, route, txnId, executeAt, deps);
- persist(node, all, route, txnId, txn, executeAt, deps, txn.execute(txnId, executeAt, null), txn.result(txnId, executeAt, null), callback);
+ persist(node, null, route, txnId, txn, executeAt, deps, txn.execute(txnId, executeAt, null), txn.result(txnId, executeAt, null), callback);
Review Comment:
yeah, could do with some more cleaning up really but just noticed this redundancy here
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1853914349
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -513,10 +513,17 @@ public enum CommitOrReadNack implements ReadData.ReadReply
* The commit has been rejected due to stale ballot.
*/
Rejected("CommitRejected"),
+
/**
* Either not committed, or not stable
*/
Insufficient("CommitInsufficient"),
+
+ /**
+ * PreApplied successfully, but the request is blocking so waiting to reply
+ */
+ Waiting("ApplyWaiting"),
Review Comment:
It's used in `ApplyThenWaitUntilApplied`
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852641441
##########
accord-core/src/main/java/accord/coordinate/Persist.java:
##########
@@ -86,12 +84,7 @@ public void onSuccess(Id from, ApplyReply reply)
if (!isDone)
{
isDone = true;
- Topologies topologies = tracker.topologies();
- Topology topology = topologies.forEpoch(txnId.epoch());
- int homeShardIndex = topology.indexForKey(route.homeKey());
- // we can persist only partially if some shards are already completed; in this case the home shard may not participate
- if (homeShardIndex >= 0)
- node.send(topology.get(homeShardIndex).nodes, to -> new InformDurable(to, topologies, route, txnId, executeAt, Majority));
+ InformDurable.informHome(node, topologies, txnId, route, executeAt);
Review Comment:
its fine, I just wanted to get your feedback on it. Redundant is fine by me, just making sure things were intentional (I have caused a lot of bugs due to refactoring missing subtle details)
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1851068651
##########
accord-core/src/main/java/accord/coordinate/ExecuteSyncPoint.java:
##########
@@ -195,6 +257,11 @@ protected void onSuccess()
trySuccess(syncPoint);
}
+ protected void sendApply(Node.Id to)
Review Comment:
I don't see it used in https://github.com/apache/cassandra/pull/3698
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -513,10 +513,17 @@ public enum CommitOrReadNack implements ReadData.ReadReply
* The commit has been rejected due to stale ballot.
*/
Rejected("CommitRejected"),
+
/**
* Either not committed, or not stable
*/
Insufficient("CommitInsufficient"),
+
+ /**
+ * PreApplied successfully, but the request is blocking so waiting to reply
+ */
+ Waiting("ApplyWaiting"),
Review Comment:
I don't see it used in https://github.com/apache/cassandra/pull/3698
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852056916
##########
accord-core/src/main/java/accord/coordinate/Persist.java:
##########
@@ -86,12 +84,7 @@ public void onSuccess(Id from, ApplyReply reply)
if (!isDone)
{
isDone = true;
- Topologies topologies = tracker.topologies();
- Topology topology = topologies.forEpoch(txnId.epoch());
- int homeShardIndex = topology.indexForKey(route.homeKey());
- // we can persist only partially if some shards are already completed; in this case the home shard may not participate
- if (homeShardIndex >= 0)
- node.send(topology.get(homeShardIndex).nodes, to -> new InformDurable(to, topologies, route, txnId, executeAt, Majority));
+ InformDurable.informHome(node, topologies, txnId, route, executeAt);
Review Comment:
it doesn't say it's unsafe, it says we might not have the information in our topologies. It's fine to inform the home shard (but perhaps redundant). I thought it was simpler to merge the different callers, but happy to split them again and introduce a `tryInformHome` (I did have this initially)
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852638457
##########
accord-core/src/main/java/accord/utils/async/AsyncChains.java:
##########
@@ -430,8 +430,10 @@ protected Cancellable start(BiConsumer<? super V, Throwable> callback)
// either the thing we start, or the thing we do in follow-up
BiConsumer<? super V, Throwable> next;
+ private final Head<?> head;
Review Comment:
its not in trunk. The patch I sent you I needed to know the state of the `allOf` to see which shard was not replying, so doing this let me get access. This was debugging code
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1852059031
##########
accord-core/src/main/java/accord/coordinate/CoordinationAdapter.java:
##########
@@ -310,14 +302,6 @@ private static class AsyncInclusiveSyncPointAdapter<U extends Unseekable> extend
protected AsyncInclusiveSyncPointAdapter() {
super();
}
-
- @Override
- protected void addOrExecuteCallback(ExecuteBlocking<U> execute, BiConsumer<? super SyncPoint<U>, Throwable> callback)
- {
- // If this is the async adapter then we want to invoke the callback immediately
Review Comment:
I just properly separated the logic for async/blocking, since we shouldn't be executing a blocking coordination act all for async, rather than simply sharing everything except deciding in `addOrExecuteCallback` whether a/sync
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1851043178
##########
accord-core/src/main/java/accord/messages/InformDurable.java:
##########
@@ -63,6 +67,22 @@ private InformDurable(TxnId txnId, Route<?> scope, long waitForEpoch, @Nullable
this.durability = durability;
}
+ public static void informHome(Node node, Topologies any, TxnId txnId, Route<?> route, Timestamp executeAt)
+ {
+ long homeEpoch = txnId.epoch();
+ Topology homeEpochTopology = any.forEpoch(homeEpoch);
+ int homeShardIndex = homeEpochTopology.indexForKey(route.homeKey());
+ if (homeShardIndex < 0)
+ {
+ homeEpochTopology = node.topology().globalForEpoch(homeEpoch);
+ homeShardIndex = homeEpochTopology.indexForKey(route.homeKey());
+ }
+
+ Shard homeShard = homeEpochTopology.get(homeShardIndex);
+ Topologies homeTopology = new Topologies.Single(any, new Topology(homeEpoch, homeShard));
+ node.send(homeShard.nodes, to -> new InformDurable(to, homeTopology, route.homeKeyOnlyRoute(), txnId, executeAt, Majority));
Review Comment:
should `Majority` be hard coded or should it be passed as a param? What about changing the method name to `informHomeMajority`
##########
accord-core/src/main/java/accord/coordinate/CoordinationAdapter.java:
##########
@@ -310,14 +302,6 @@ private static class AsyncInclusiveSyncPointAdapter<U extends Unseekable> extend
protected AsyncInclusiveSyncPointAdapter() {
super();
}
-
- @Override
- protected void addOrExecuteCallback(ExecuteBlocking<U> execute, BiConsumer<? super SyncPoint<U>, Throwable> callback)
- {
- // If this is the async adapter then we want to invoke the callback immediately
Review Comment:
what is the reason to make this async rather than immediate?
##########
accord-core/src/main/java/accord/messages/InformDurable.java:
##########
@@ -63,6 +67,22 @@ private InformDurable(TxnId txnId, Route<?> scope, long waitForEpoch, @Nullable
this.durability = durability;
}
+ public static void informHome(Node node, Topologies any, TxnId txnId, Route<?> route, Timestamp executeAt)
+ {
+ long homeEpoch = txnId.epoch();
+ Topology homeEpochTopology = any.forEpoch(homeEpoch);
+ int homeShardIndex = homeEpochTopology.indexForKey(route.homeKey());
+ if (homeShardIndex < 0)
+ {
+ homeEpochTopology = node.topology().globalForEpoch(homeEpoch);
+ homeShardIndex = homeEpochTopology.indexForKey(route.homeKey());
+ }
+
+ Shard homeShard = homeEpochTopology.get(homeShardIndex);
+ Topologies homeTopology = new Topologies.Single(any, new Topology(homeEpoch, homeShard));
+ node.send(homeShard.nodes, to -> new InformDurable(to, homeTopology, route.homeKeyOnlyRoute(), txnId, executeAt, Majority));
Review Comment:
in `MaybeRecover` we call `accord.primitives.Status.Durability#isDurable` which is
```
public boolean isDurable()
{
return this == Majority || this == Universal;
}
```
so based off what we know, it "could" be `Universal` as well?
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -513,10 +513,17 @@ public enum CommitOrReadNack implements ReadData.ReadReply
* The commit has been rejected due to stale ballot.
*/
Rejected("CommitRejected"),
+
/**
* Either not committed, or not stable
*/
Insufficient("CommitInsufficient"),
+
+ /**
+ * PreApplied successfully, but the request is blocking so waiting to reply
+ */
+ Waiting("ApplyWaiting"),
Review Comment:
I don't see this sent in this patch, is this done in C*?
##########
accord-core/src/main/java/accord/coordinate/MaybeRecover.java:
##########
@@ -108,7 +108,7 @@ protected void onDone(Success success, Throwable fail)
if (hasMadeProgress(full))
{
if (full.durability.isDurable())
- node.send(topologies.forEpoch(txnId.epoch()).forKey(route.homeKey()).nodes, to -> new InformDurable(to, topologies, route, txnId, full.executeAtIfKnown(), full.durability));
+ InformDurable.informHome(node, topologies, txnId, route, full.executeAtIfKnown());
Review Comment:
commented in `InformDurable.informHome` but durability "could" be `Universal` yet this method hard codes `Majority`
##########
accord-core/src/main/java/accord/coordinate/CoordinationAdapter.java:
##########
@@ -219,18 +224,17 @@ public void stabilise(Node node, Topologies any, FullRoute<?> route, Ballot ball
@Override
public void execute(Node node, Topologies any, FullRoute<?> route, ExecutePath path, TxnId txnId, Txn txn, Timestamp executeAt, Deps deps, BiConsumer<? super SyncPoint<U>, Throwable> callback)
{
- Topologies all = forExecution(node, route, txnId, executeAt, deps);
- persist(node, all, route, txnId, txn, executeAt, deps, txn.execute(txnId, executeAt, null), txn.result(txnId, executeAt, null), callback);
+ persist(node, null, route, txnId, txn, executeAt, deps, txn.execute(txnId, executeAt, null), txn.result(txnId, executeAt, null), callback);
Review Comment:
just double checking, this is because every impl recalculates `all`, so just avoid double effort?
##########
accord-core/src/main/java/accord/coordinate/CoordinationAdapter.java:
##########
@@ -219,18 +224,17 @@ public void stabilise(Node node, Topologies any, FullRoute<?> route, Ballot ball
@Override
public void execute(Node node, Topologies any, FullRoute<?> route, ExecutePath path, TxnId txnId, Txn txn, Timestamp executeAt, Deps deps, BiConsumer<? super SyncPoint<U>, Throwable> callback)
{
- Topologies all = forExecution(node, route, txnId, executeAt, deps);
- persist(node, all, route, txnId, txn, executeAt, deps, txn.execute(txnId, executeAt, null), txn.result(txnId, executeAt, null), callback);
+ persist(node, null, route, txnId, txn, executeAt, deps, txn.execute(txnId, executeAt, null), txn.result(txnId, executeAt, null), callback);
}
@Override
- public void persist(Node node, Topologies any, FullRoute<?> route, Participants<?> participants, TxnId txnId, Txn txn, Timestamp executeAt, Deps deps, Writes writes, Result result, BiConsumer<? super SyncPoint<U>, Throwable> callback)
+ public void persist(Node node, Topologies ignore, FullRoute<?> route, Participants<?> participants, TxnId txnId, Txn txn, Timestamp executeAt, Deps deps, Writes writes, Result result, BiConsumer<? super SyncPoint<U>, Throwable> callback)
{
Topologies all = forExecution(node, route, txnId, executeAt, deps);
invokeSuccess(node, route, txnId, txn, deps, callback);
new PersistSyncPoint(node, all, txnId, route, txn, executeAt, deps, writes, result)
- .start(Apply.FACTORY, Maximal, any, writes, result);
+ .start(Apply.FACTORY, Maximal, all, writes, result);
Review Comment:
for me: lgtm
##########
accord-core/src/main/java/accord/coordinate/Persist.java:
##########
@@ -86,12 +84,7 @@ public void onSuccess(Id from, ApplyReply reply)
if (!isDone)
{
isDone = true;
- Topologies topologies = tracker.topologies();
- Topology topology = topologies.forEpoch(txnId.epoch());
- int homeShardIndex = topology.indexForKey(route.homeKey());
- // we can persist only partially if some shards are already completed; in this case the home shard may not participate
- if (homeShardIndex >= 0)
- node.send(topology.get(homeShardIndex).nodes, to -> new InformDurable(to, topologies, route, txnId, executeAt, Majority));
+ InformDurable.informHome(node, topologies, txnId, route, executeAt);
Review Comment:
we will now send as we fall back to the global topology yet the previous comment implies this isn't safe?
##########
accord-core/src/main/java/accord/coordinate/ExecuteSyncPoint.java:
##########
@@ -195,6 +257,11 @@ protected void onSuccess()
trySuccess(syncPoint);
}
+ protected void sendApply(Node.Id to)
Review Comment:
this appears to be dead code, is this used by C*?
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1851034163
##########
accord-core/src/main/java/accord/utils/async/AsyncChains.java:
##########
@@ -430,8 +430,10 @@ protected Cancellable start(BiConsumer<? super V, Throwable> callback)
// either the thing we start, or the thing we do in follow-up
BiConsumer<? super V, Throwable> next;
+ private final Head<?> head;
Review Comment:
you can revert this, I did this to aid in debugger support
--
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] Fix infinite loop and notify progress log of sync point durability while waiting to apply [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #141:
URL: https://github.com/apache/cassandra-accord/pull/141#discussion_r1868307128
##########
accord-core/src/main/java/accord/coordinate/Persist.java:
##########
@@ -83,18 +78,12 @@ public void onSuccess(Id from, ApplyReply reply)
default: throw new IllegalStateException();
case Redundant:
case Applied:
- persistedOn.add(from);
Review Comment:
for me: confirmed this is existing dead code, so this patch just cleans it up
--
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