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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
