ifesdjeen commented on code in PR #237:
URL: https://github.com/apache/cassandra-accord/pull/237#discussion_r2269870996
##########
accord-core/src/main/java/accord/local/DepsCalculator.java:
##########
@@ -137,13 +135,14 @@ public Deps calculate(SafeCommandStore safeStore, TxnId
txnId, Participants<?> t
TxnId maxRedundantBefore = redundant.maxTxnId(null);
if (maxRedundantBefore != null &&
maxRedundantBefore.compareTo(executeAt) >= 0)
{
- Invariants.require(maxRedundantBefore.is(ExclusiveSyncPoint));
+ Invariants.require(maxRedundantBefore.isSyncPoint());
return null;
}
}
// NOTE: ExclusiveSyncPoint *relies* on STARTED_BEFORE to ensure it
reports a dependency on *every* earlier TxnId that may execute (before or after
it).
MinDependencyCalculator minDepCalc = null;
+ // the main different between RX and RV is whether we apply this
filtering
Review Comment:
nit: difference?
##########
accord-core/src/main/java/accord/local/RedundantBefore.java:
##########
@@ -360,7 +366,7 @@ private static Timestamp
maybeClearStaleUntilAtLeast(Timestamp staleUntilAtLeast
public Bounds with(TxnId newBound, SomeStatus addStatus)
{
// TODO (desired): introduce special-cased faster merge for adding
a single value
- return merge(range, this, new Bounds(range, Long.MIN_VALUE,
Long.MAX_VALUE, new TxnId[] { newBound }, new short[] { addStatus.encoded },
null));
+ return merge(range, this, new Bounds(range, Long.MIN_VALUE,
Long.MAX_VALUE, new TxnId[] { newBound }, new short[] {
(short)addStatus.encoded }, null));
Review Comment:
Don't we want to mask here while down casting?
##########
accord-core/src/main/java/accord/impl/progresslog/DefaultProgressLog.java:
##########
@@ -219,60 +223,26 @@ public void update(SafeCommandStore safeStore, TxnId
txnId, Command before, Comm
if (!txnId.isVisible())
return;
- TxnState state = null;
Route<?> beforeRoute = before.route();
Route<?> afterRoute = after.route();
- if (force || (afterRoute != null && beforeRoute == null) ||
(after.durability().isDurableOrInvalidated() &&
!before.durability().isDurableOrInvalidated()))
- state = updateOrInitialiseHomeState(safeStore, after, get(txnId));
-
SaveStatus beforeSaveStatus = before.saveStatus();
SaveStatus afterSaveStatus = after.saveStatus();
- if (beforeSaveStatus == afterSaveStatus && !force)
- return;
-
- if (state == null)
- state = get(txnId);
-
- if (state == null)
- return;
- state.waiting().record(this, afterSaveStatus);
- if (state.isHomeInitialised())
- updateHomeState(safeStore, state, before, after);
- }
-
- @Override
- public void decided(SafeCommandStore safeStore, TxnId txnId)
- {
- TxnState state = get(txnId);
- if (state != null && state.isHomeInitialised())
- state.home().atLeast(safeStore, this, Decided, NoneExpected);
- }
-
- private void updateHomeState(SafeCommandStore safeStore, TxnState state,
Command before, Command after)
- {
- switch (after.saveStatus())
+ boolean updateWaiting = force || beforeSaveStatus != afterSaveStatus;
+ boolean updateHome = updateWaiting || (afterRoute != null &&
beforeRoute == null) || !before.durability().isAtLeast(after.durability());
Review Comment:
should the third condition be
`after.durability().isAtLeast(before.durability()`?
##########
accord-core/src/main/java/accord/local/RedundantBefore.java:
##########
@@ -351,7 +357,7 @@ private static Timestamp
maybeClearStaleUntilAtLeast(Timestamp staleUntilAtLeast
{
for (int i = 0 ; staleUntilAtLeast != null && i < bounds.length &&
bounds[i].compareTo(staleUntilAtLeast) > 0 ; ++i)
{
- if (any(statuses[i], PRE_BOOTSTRAP))
+ if (any(0xFFFF & statuses[i], PRE_BOOTSTRAP))
Review Comment:
could you explain why mask it if it's short and we have an overload for
short?
##########
accord-core/src/main/java/accord/local/Commands.java:
##########
@@ -1097,38 +1100,40 @@ public static Command setDurability(SafeCommandStore
safeStore, SafeCommand safe
if (command.is(Truncated))
return command;
- if (command.durability().compareTo(durability) >= 0)
+ Durability oldDurability = command.durability();
+ Durability newDurability = oldDurability.mergeMax(durability);
+ if (newDurability == oldDurability)
return command;
Command updated = supplementParticipants(safeStore, safeCommand,
participants);
participants = updated.participants();
if (executeAt != null && command.status().hasBeen(Committed) &&
!command.executeAt().equals(executeAt))
safeStore.agent().onInconsistentTimestamp(command,
command.asCommitted().executeAt(), executeAt);
- if (command.durability().compareTo(durability) < 0)
+ updated = safeCommand.update(safeStore,
updated.updateDurability(newDurability));
+ TxnId txnId = command.txnId();
+ DependencyElision updates = OFF;
+ if (newDurability.isDurable() && !oldDurability.isDurable()) updates =
IF_DURABLY_PREAPPLIED;
+ else if (newDurability.isDurablyCommitted() &&
!durability.isDurablyCommitted()) updates = IF_DURABLY_COMMITTED;
Review Comment:
Should `durability.isDurablyCommitted()` actually be `oldDurability`?
##########
accord-core/src/main/java/accord/local/Commands.java:
##########
@@ -1097,38 +1100,40 @@ public static Command setDurability(SafeCommandStore
safeStore, SafeCommand safe
if (command.is(Truncated))
return command;
- if (command.durability().compareTo(durability) >= 0)
+ Durability oldDurability = command.durability();
Review Comment:
Should we rename `durability` in `setDurability` to `durabilityUpdate` or
`durabilityChange` ?
##########
accord-core/src/main/java/accord/local/cfk/CommandsForKey.java:
##########
@@ -1604,18 +1639,21 @@ public CommandsForKey updateUniqueHlc(long minUniqueHlc)
if (maxUniqueHlc <= bounds.gcBefore.hlc() &&
bounds.gcBefore.is(HLC_BOUND))
return this;
- return new CommandsForKey(key, bounds, byId, minUndecidedById,
maxAppliedPreBootstrapWriteById, committedByExecuteAt,
maxAppliedWriteByExecuteAt, minUniqueHlc, loadingPruned, prunedBeforeById,
unmanageds);
+ return new CommandsForKey(key, bounds, byId, minUndecidedById,
maxAppliedPreBootstrapWriteById, committedByExecuteAt,
maxAppliedWriteByExecuteAt, minUniqueHlc, loadingPruned, prunedBeforeById,
unmanageds, true);
}
- public CommandsForKey setDurable(TxnId txnId)
+ public CommandsForKey setDurable(TxnId txnId, Durability durability)
{
TxnInfo txn = get(txnId);
- if (txn == null || !txn.is(APPLIED_NOT_DURABLE))
+ if (txn == null || !durability.isDurablyCommitted())
Review Comment:
I might be misunderstanding: do we _not_ want to set durable in CFK if not
yet durably committed? Should we even be calling this method then?
--
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]
