dcapwell commented on code in PR #50:
URL: https://github.com/apache/cassandra-accord/pull/50#discussion_r1227213481
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -52,6 +52,7 @@
import accord.local.Listeners;
import accord.local.NodeTimeService;
import accord.local.PreLoadContext;
+import accord.local.RedundantBefore;
Review Comment:
```
> Task :accord-core:checkstyleMain FAILED
[ant:checkstyle] [ERROR]
/Users/dcapwell/src/github/apache/cassandra-accord/prs/trunk/accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:55:8:
Unused import - accord.local.RedundantBefore. [UnusedImports]
```
##########
accord-core/src/main/java/accord/local/SafeCommand.java:
##########
@@ -131,14 +143,27 @@ public Command.Executed preapplied(CommonAttributes
attrs, Timestamp executeAt,
return update(Command.preapplied(current(), attrs, executeAt,
waitingOn, writes, result));
}
+ public Command.Executed applying()
+ {
+ return update(Command.applying(current().asExecuted()));
+ }
+
public Command.Executed applied()
{
return update(Command.applied(current().asExecuted()));
}
- public Command.NotWitnessed notWitnessed()
+ public Command.NotDefined uninitialised()
{
Invariants.checkArgument(current() == null);
- return update(Command.NotWitnessed.notWitnessed(txnId));
+ return update(Command.NotDefined.uninitialised(txnId));
+ }
+
+ public Command initialise()
+ {
+ Command current = current();
+ if (current.saveStatus() != Uninitialised)
+ return current;
+ return update(Command.NotDefined.notDefined(current,
current.promised()));
Review Comment:
the caller is
```
// ensure we have a record to work with later; otherwise may
think has been truncated
blockedBy.initialise();
```
so I guess we do want `NotDefined(promised = null)`, so the distinction is
getting blurred to me... so if we depend on a txn, or saw it as invalidated
before we saw its definition, then we are `NotDefined`... but how did we learn
about this txn id if we didn't learn it from a peer?
##########
accord-core/src/main/java/accord/api/ConfigurationService.java:
##########
@@ -111,13 +114,35 @@ public static EpochReady done(long epoch)
*
* TODO (required): document what this Future represents, or maybe
refactor it away - only used for testing
*/
- AsyncResult<Void> onTopologyUpdate(Topology topology);
+ AsyncResult<Void> onTopologyUpdate(Topology topology, boolean
startSync);
/**
* Called when accord data associated with a superseded epoch has been
sync'd from previous replicas.
* This should be invoked on each replica once EpochReady.coordination
has returned on a replica.
+ *
+ * Once a quorum of these notifications have been received, no new
TxnId may be executed in this epoch
+ * (though this is not a transitive property; earlier epochs may yet
agree to execute TxnId if they have not been sync'd)
+ */
+ void onRemoteSyncComplete(Node.Id node, long epoch);
+
+ /**
+ * Called when the configuration service is meant to truncate it's
topology data up to (but not including)
+ * the given epoch
+ */
+ void truncateTopologyUntil(long epoch);
+
+ /**
+ * Called when no new TxnId may be agreed with an epoch less than or
equal to the provided one.
+ * This means future epochs are now aware of all TxnId with this epoch
or earlier that may be executed
+ * on this range.
+ */
+ void onEpochClosed(Ranges ranges, long epoch);
+
+ /**
+ * Called when all TxnId with an epoch equal to or before this that
interact with this range have been executed,
+ * in whatever epoch they execute in. Once the whole range is covered
this epoch is redundant, and may be cleaned up.
*/
- void onEpochSyncComplete(Node.Id node, long epoch);
+ void onEpochRedundant(Ranges ranges, long epoch);
Review Comment:
for me: in burn test called by
`accord.coordinate.CoordinateShardDurable#onSuccess`
##########
accord-core/src/main/java/accord/local/Command.java:
##########
@@ -564,6 +542,68 @@ public PartialTxn partialTxn()
{
return null;
}
+
+ private static SaveStatus initialise(SaveStatus saveStatus)
+ {
+ return saveStatus == Uninitialised ? SaveStatus.NotDefined :
saveStatus;
Review Comment:
im confused by this... the only caller is
`accord.local.Command.NotDefined#updateAttributes` which takes the current
`saveStatus` and supplies it to this function.... `NotDefined` should only
have 1 of 2 states: `Uninitialised` and `NotDefined`...
If we say that `Uninitialised` maps to `NotDefined`, then isn't this
function equal to just setting `NotDefined`?
##########
accord-core/src/main/java/accord/api/ConfigurationService.java:
##########
@@ -111,13 +114,35 @@ public static EpochReady done(long epoch)
*
* TODO (required): document what this Future represents, or maybe
refactor it away - only used for testing
*/
- AsyncResult<Void> onTopologyUpdate(Topology topology);
+ AsyncResult<Void> onTopologyUpdate(Topology topology, boolean
startSync);
/**
* Called when accord data associated with a superseded epoch has been
sync'd from previous replicas.
* This should be invoked on each replica once EpochReady.coordination
has returned on a replica.
+ *
+ * Once a quorum of these notifications have been received, no new
TxnId may be executed in this epoch
+ * (though this is not a transitive property; earlier epochs may yet
agree to execute TxnId if they have not been sync'd)
+ */
+ void onRemoteSyncComplete(Node.Id node, long epoch);
+
+ /**
+ * Called when the configuration service is meant to truncate it's
topology data up to (but not including)
+ * the given epoch
+ */
+ void truncateTopologyUntil(long epoch);
+
+ /**
+ * Called when no new TxnId may be agreed with an epoch less than or
equal to the provided one.
+ * This means future epochs are now aware of all TxnId with this epoch
or earlier that may be executed
+ * on this range.
+ */
+ void onEpochClosed(Ranges ranges, long epoch);
Review Comment:
for me: in burn test called in
`accord.coordinate.CoordinateSyncPoint#onPreAccepted`
##########
accord-core/src/main/java/accord/local/SafeCommand.java:
##########
@@ -131,14 +143,27 @@ public Command.Executed preapplied(CommonAttributes
attrs, Timestamp executeAt,
return update(Command.preapplied(current(), attrs, executeAt,
waitingOn, writes, result));
}
+ public Command.Executed applying()
+ {
+ return update(Command.applying(current().asExecuted()));
+ }
+
public Command.Executed applied()
{
return update(Command.applied(current().asExecuted()));
}
- public Command.NotWitnessed notWitnessed()
+ public Command.NotDefined uninitialised()
{
Invariants.checkArgument(current() == null);
- return update(Command.NotWitnessed.notWitnessed(txnId));
+ return update(Command.NotDefined.uninitialised(txnId));
+ }
+
+ public Command initialise()
+ {
+ Command current = current();
+ if (current.saveStatus() != Uninitialised)
+ return current;
+ return update(Command.NotDefined.notDefined(current,
current.promised()));
Review Comment:
`saveStatus == Uninitialised`, so wouldn't this return `NotDefined` with a
`null` for promised (if we saw an invalidate we transitioned to `NotDefined`;
so wouldn't that mean this returns a `NotDefined` with the same semantics as
`Uninitialised`?
##########
accord-core/src/main/java/accord/coordinate/Truncated.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package accord.coordinate;
+
+import javax.annotation.Nullable;
+
+import accord.api.RoutingKey;
+import accord.primitives.TxnId;
+
+/**
+ * Thrown when a transaction exceeds its specified timeout for obtaining a
result for a client
Review Comment:
think this javadoc was copy/pasted? `Truncated` isn't what we throw on
`Timeout`
##########
accord-core/src/main/java/accord/local/SafeCommand.java:
##########
@@ -131,14 +143,27 @@ public Command.Executed preapplied(CommonAttributes
attrs, Timestamp executeAt,
return update(Command.preapplied(current(), attrs, executeAt,
waitingOn, writes, result));
}
+ public Command.Executed applying()
+ {
+ return update(Command.applying(current().asExecuted()));
+ }
+
public Command.Executed applied()
{
return update(Command.applied(current().asExecuted()));
}
- public Command.NotWitnessed notWitnessed()
+ public Command.NotDefined uninitialised()
{
Invariants.checkArgument(current() == null);
- return update(Command.NotWitnessed.notWitnessed(txnId));
+ return update(Command.NotDefined.uninitialised(txnId));
+ }
+
+ public Command initialise()
+ {
+ Command current = current();
+ if (current.saveStatus() != Uninitialised)
+ return current;
+ return update(Command.NotDefined.notDefined(current,
current.promised()));
Review Comment:
Looking at `accord.local.SafeCommandStore#get(accord.primitives.TxnId,
accord.primitives.Unseekables<?>)` I guess we can do the following...
```
Applied -> Truncated -> Uninitialised // caused by truncate
```
And ``accord.impl.AbstractSafeCommandStore#get says that we asked for a txn,
but there was no known txn, so we return `Uninitialised`... So this happens
when we see an unknow txn for the first time, or it was purged... but
NotDefined is for txn we learn from peers... which feels confusing.
##########
accord-core/src/main/java/accord/local/SaveStatus.java:
##########
@@ -35,30 +35,41 @@
*/
public enum SaveStatus
{
- NotWitnessed (Status.NotWitnessed),
+ // TODO (expected): erase Uninitialised in Context once command finishes
+ // TODO (expected): we can use Uninitialised in several places to
simplify/better guarantee correct behaviour with truncation
+ Uninitialised (Status.NotDefined),
+ NotDefined (Status.NotDefined),
Review Comment:
`NotDefined` seems to be "command was `Uninitialised`, but invalidation
informed us that this is being invalidated"... So maybe
`UninitialisedPendingInvalidation`? Right now the 2 states are more confusing
and harder to keep track in my head... as they feel like the 2 names mean the
same thing...
--
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]
