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 2025/01/06 12:57:21 UTC
[PR] Accord: Bootstrap should detect insufficient source [cassandra-accord]
belliottsmith opened a new pull request, #154:
URL: https://github.com/apache/cassandra-accord/pull/154
(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] Accord: Bootstrap should detect insufficient source [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith merged PR #154:
URL: https://github.com/apache/cassandra-accord/pull/154
--
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] Accord: Bootstrap should detect insufficient source [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #154:
URL: https://github.com/apache/cassandra-accord/pull/154#discussion_r1914988778
##########
accord-core/src/main/java/accord/local/cfk/Utils.java:
##########
@@ -261,6 +262,28 @@ static TxnId[] removePrunedAdditions(TxnId[] additions, int additionCount, TxnId
return prunedIds;
}
+ static TxnId[] removeUnmanaged(TxnId[] ids)
+ {
+ int count = 0;
Review Comment:
I will make a note to consider this more later. My intuition is that counting will have negligible cost, as these arrays should normally be small. But equally in C* we expect to have very few unmanaged transactions.
--
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] Accord: Bootstrap should detect insufficient source [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #154:
URL: https://github.com/apache/cassandra-accord/pull/154#discussion_r1915010833
##########
accord-core/src/main/java/accord/local/cfk/Utils.java:
##########
@@ -261,6 +262,28 @@ static TxnId[] removePrunedAdditions(TxnId[] additions, int additionCount, TxnId
return prunedIds;
}
+ static TxnId[] removeUnmanaged(TxnId[] ids)
+ {
+ int count = 0;
Review Comment:
Hmm, actually no I think the current approach is optimal in the case we expect it to be mostly managed, as we don't copy anything if we don't remove anything?
--
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] Accord: Bootstrap should detect insufficient source [cassandra-accord]
Posted by "ifesdjeen (via GitHub)" <gi...@apache.org>.
ifesdjeen commented on code in PR #154:
URL: https://github.com/apache/cassandra-accord/pull/154#discussion_r1908499629
##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -285,16 +286,30 @@ protected void unsafeUpsertRedundantBefore(RedundantBefore addRedundantBefore)
/**
* This method may be invoked on a non-CommandStore thread
*/
- protected synchronized void unsafeSetSafeToRead(NavigableMap<Timestamp, Ranges> newSafeToRead)
+ final void unsafeSetSafeToRead(NavigableMap<Timestamp, Ranges> newSafeToRead)
{
this.safeToRead = newSafeToRead;
}
- protected void unsafeSetBootstrapBeganAt(NavigableMap<TxnId, Ranges> newBootstrapBeganAt)
+ protected void loadSafeToRead(NavigableMap<Timestamp, Ranges> newSafeToRead)
+ {
+ Invariants.checkState(safeToRead == null || safeToRead.equals(ImmutableSortedMap.of(Timestamp.NONE, Ranges.EMPTY)));
Review Comment:
nit: Been meaning to do this for a while, but never got to: should we put default values for safe to read and bootstrap began at somewhere handy? I keep repeating them here and there.
##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -311,8 +326,34 @@ protected void updateMaxConflicts(Command prev, Command updated)
if (executeAt == null) return;
if (prev != null && prev.executeAt() != null && prev.executeAt().compareTo(executeAt) >= 0) return;
-
MaxConflicts updatedMaxConflicts = maxConflicts.update(updated.participants().hasTouched(), executeAt);
+ updateMaxConflicts(executeAt, updatedMaxConflicts);
+ }
+
+ protected void updateMaxConflicts(Ranges ranges, Timestamp executeAt)
+ {
+ updateMaxConflicts(executeAt, maxConflicts.update(ranges, executeAt));
+ }
+
+ protected void updateMaxConflicts(NavigableMap<? extends Timestamp, Ranges> map)
+ {
+ Timestamp max = Timestamp.NONE;
+ MaxConflicts updated = maxConflicts;
+ for (Map.Entry<? extends Timestamp, Ranges> e : map.entrySet())
+ {
+ Timestamp at = e.getKey();
+ if (at.compareTo(Timestamp.NONE) > 0)
+ {
+ updated = updated.update(e.getValue(), at);
+ max = Timestamp.max(max, at);
Review Comment:
I might be missing something, but do we have a guarantee that existing `maxConflicts` does not have a higher timestamp? In other words, we collect max timestamp only from the provided map, but not from the existing `maxConflict`
##########
accord-core/src/main/java/accord/local/cfk/CommandsForKey.java:
##########
@@ -1433,52 +1497,58 @@ private CommandsForKeyUpdate update(InternalStatus newStatus, Command next, bool
return this; // if outOfRange we only need to maintain any existing records; if none, don't update
pos = -1 - pos;
- if (isOutOfRange) result = insertOrUpdateOutOfRange(pos, txnId, null, newStatus, isDurable, mayExecute, wasPruned, loadingAsPrunedFor);
- else if (newStatus.hasDeps() && !wasPruned) result = insert(pos, txnId, newStatus, isDurable, mayExecute, next);
- else result = insert(pos, txnId, TxnInfo.create(txnId, newStatus, isDurable, mayExecute, next), wasPruned, loadingAsPrunedFor);
+ if (isOutOfRange) result = insertOrUpdateOutOfRange(pos, txnId, null, newStatus, mayExecute, updated, wasPruned, loadingAsPrunedFor);
+ else if (newStatus.hasDeps() && !wasPruned) result = insert(pos, txnId, newStatus, mayExecute, updated);
+ else result = insert(pos, txnId, TxnInfo.create(txnId, newStatus, mayExecute, updated), wasPruned, loadingAsPrunedFor);
}
else
{
// update
TxnInfo cur = byId[pos];
-
if (cur != null)
{
int c = cur.compareTo(newStatus);
- if (c >= 0)
+ if (c > 0)
{
- if (c > 0)
- {
- if (!(newStatus == PREACCEPTED_OR_ACCEPTED_INVALIDATE && cur.is(ACCEPTED) && next.acceptedOrCommitted().compareTo(cur.ballot()) > 0))
- return this;
- }
- else
- {
- if (!newStatus.hasExecuteAtAndDeps)
- return this;
+ // newStatus moves us backwards; we only permit this for (Pre)?(Not)?Accepted states
+ if (cur.compareTo(COMMITTED) >= 0 || newStatus.compareTo(PREACCEPTED_OR_ACCEPTED_INVALIDATE) < 0)
+ return this;
- if (next.acceptedOrCommitted().compareTo(cur.ballot()) <= 0)
- return this;
- }
+ // and only when the new ballot is strictly greater
+ if (updated.acceptedOrCommitted().compareTo(cur.ballot()) <= 0)
+ return this;
+ }
+ else if (c == 0)
+ {
+ // we're updating to the same state; we only do this with a strictly greater ballot;
+ // even so, if we have no executeAt or deps there's nothing to record
+ if (updated.acceptedOrCommitted().compareTo(cur.ballot()) <= 0 && !newStatus.hasExecuteAtOrDeps())
Review Comment:
don't we need to consider `(!newStatus.hasExecuteAtAndDeps)` here?
##########
accord-core/src/main/java/accord/local/cfk/CommandsForKey.java:
##########
@@ -1819,13 +1893,28 @@ public CommandsForKeyUpdate withRedundantBeforeAtLeast(RedundantBefore.Entry new
*/
public CommandsForKeyUpdate withRedundantBeforeAtLeast(RedundantBefore.Entry newBoundsInfo, boolean force)
{
- // TODO (required): handle receiving an entry from the past, e.g. on reload (OR expunge all CFK on restart)
- if (!force && newBoundsInfo.gcBefore.equals(boundsInfo.gcBefore)
- && newBoundsInfo.bootstrappedAt.equals(boundsInfo.bootstrappedAt)
- && newBoundsInfo.locallyDecidedAndAppliedOrInvalidatedBefore.equals(boundsInfo.locallyDecidedAndAppliedOrInvalidatedBefore)
- && newBoundsInfo.endOwnershipEpoch == boundsInfo.endOwnershipEpoch)
+ // we can't let HLC epoch go backwards as this breaks assumptions around maxHlc tracking
Review Comment:
I could not find any usages where `force` would be passed as `true`, do we have any plans for this? If not, maybe worth considering to remove to simplify logic a bit?
##########
accord-core/src/main/java/accord/local/cfk/Utils.java:
##########
@@ -261,6 +262,28 @@ static TxnId[] removePrunedAdditions(TxnId[] additions, int additionCount, TxnId
return prunedIds;
}
+ static TxnId[] removeUnmanaged(TxnId[] ids)
+ {
+ int count = 0;
Review Comment:
Nit: if we have mostly-managed, we could just pre-allocate an array with same size as ids, and copy as we go, then system.arrays copy the `count` elements. Feels it might be a bit more efficient than iterating twice.
--
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] Accord: Bootstrap should detect insufficient source [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #154:
URL: https://github.com/apache/cassandra-accord/pull/154#discussion_r1914963479
##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -311,8 +326,34 @@ protected void updateMaxConflicts(Command prev, Command updated)
if (executeAt == null) return;
if (prev != null && prev.executeAt() != null && prev.executeAt().compareTo(executeAt) >= 0) return;
-
MaxConflicts updatedMaxConflicts = maxConflicts.update(updated.participants().hasTouched(), executeAt);
+ updateMaxConflicts(executeAt, updatedMaxConflicts);
+ }
+
+ protected void updateMaxConflicts(Ranges ranges, Timestamp executeAt)
+ {
+ updateMaxConflicts(executeAt, maxConflicts.update(ranges, executeAt));
+ }
+
+ protected void updateMaxConflicts(NavigableMap<? extends Timestamp, Ranges> map)
+ {
+ Timestamp max = Timestamp.NONE;
+ MaxConflicts updated = maxConflicts;
+ for (Map.Entry<? extends Timestamp, Ranges> e : map.entrySet())
+ {
+ Timestamp at = e.getKey();
+ if (at.compareTo(Timestamp.NONE) > 0)
+ {
+ updated = updated.update(e.getValue(), at);
+ max = Timestamp.max(max, at);
Review Comment:
I don't follow. The goal here is to ensure all of the timestamps in the map are represented in `MaxConflicts`
--
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