dcapwell commented on code in PR #50:
URL: https://github.com/apache/cassandra-accord/pull/50#discussion_r1262944112
##########
accord-core/src/main/java/accord/topology/TopologyManager.java:
##########
@@ -59,40 +68,77 @@ static class EpochState
private final Topology global;
private final Topology local;
private final QuorumTracker syncTracker;
- private boolean syncComplete;
- private boolean prevSynced;
+ private final BitSet curShardSyncComplete;
+ private final Ranges newRanges;
+ private Ranges curSyncComplete, prevSyncComplete, syncComplete;
+ Ranges closed = Ranges.EMPTY, complete = Ranges.EMPTY;
- EpochState(Id node, Topology global, TopologySorter sorter, boolean
syncComplete, boolean prevSynced)
+ EpochState(Id node, Topology global, TopologySorter sorter, Ranges
prevRanges, Ranges prevSyncComplete)
{
this.self = node;
this.global = checkArgument(global, !global.isSubset());
this.local = global.forNode(node).trim();
Invariants.checkArgument(!global().isSubset());
- // TODO: can we just track sync for local ranges here?
+ this.curShardSyncComplete = new BitSet(global.shards.length);
if (global().size() > 0)
- {
this.syncTracker = new QuorumTracker(new Single(sorter,
global()));
- this.syncComplete = syncComplete;
- this.prevSynced = prevSynced;
+ else
+ this.syncTracker = null; this.newRanges =
global.ranges.subtract(prevRanges);
+ this.prevSyncComplete = newRanges.with(prevSyncComplete);
+ this.curSyncComplete = this.syncComplete = newRanges;
+ }
+
+ boolean markPrevSynced(Ranges newPrevSyncComplete)
+ {
+ if (prevSyncComplete.containsAll(newPrevSyncComplete))
+ return false;
+
Invariants.checkState(newPrevSyncComplete.containsAll(prevSyncComplete));
Review Comment:
```suggestion
Invariants.checkState(newPrevSyncComplete.containsAll(prevSyncComplete),
"Expected %s to contain all ranges in %s; but did not", newPrevSyncComplete,
prevSyncComplete);
```
##########
accord-core/src/main/java/accord/topology/TopologyManager.java:
##########
@@ -59,40 +68,77 @@ static class EpochState
private final Topology global;
private final Topology local;
private final QuorumTracker syncTracker;
- private boolean syncComplete;
- private boolean prevSynced;
+ private final BitSet curShardSyncComplete;
+ private final Ranges newRanges;
+ private Ranges curSyncComplete, prevSyncComplete, syncComplete;
+ Ranges closed = Ranges.EMPTY, complete = Ranges.EMPTY;
- EpochState(Id node, Topology global, TopologySorter sorter, boolean
syncComplete, boolean prevSynced)
+ EpochState(Id node, Topology global, TopologySorter sorter, Ranges
prevRanges, Ranges prevSyncComplete)
{
this.self = node;
this.global = checkArgument(global, !global.isSubset());
this.local = global.forNode(node).trim();
Invariants.checkArgument(!global().isSubset());
- // TODO: can we just track sync for local ranges here?
+ this.curShardSyncComplete = new BitSet(global.shards.length);
if (global().size() > 0)
- {
this.syncTracker = new QuorumTracker(new Single(sorter,
global()));
- this.syncComplete = syncComplete;
- this.prevSynced = prevSynced;
+ else
+ this.syncTracker = null; this.newRanges =
global.ranges.subtract(prevRanges);
Review Comment:
need newline; added to my feedback branch
##########
accord-core/src/main/java/accord/topology/TopologyManager.java:
##########
@@ -212,32 +250,87 @@ public void syncComplete(Id node, long epoch)
checkArgument(epoch > 0);
if (epoch > currentEpoch)
{
- int idx = (int) (epoch - (1 + currentEpoch));
- for (int i=pendingSyncComplete.size(); i<=idx; i++)
- pendingSyncComplete.add(new HashSet<>());
+ pending(epoch).syncComplete.add(node);
+ }
+ else
+ {
+ int i = indexOf(epoch);
+ if (i < 0 || !epochs[i].recordSyncComplete(node))
+ return;
+
+ while (--i >= 0 && epochs[i].markPrevSynced(epochs[i +
1].syncComplete)) {}
Review Comment:
this doesn't feel right to me in the case of ranges being removed.
If I am reading this correctly, we are saying
`epoch(42).markPrevSynced(epoch(43).syncComplete)`, which then leads to
```
boolean markPrevSynced(Ranges newPrevSyncComplete)
{
if (prevSyncComplete.containsAll(newPrevSyncComplete))
return false;
Invariants.checkState(newPrevSyncComplete.containsAll(prevSyncComplete),
"Expected %s to contain all ranges in %s; but did not", newPrevSyncComplete,
prevSyncComplete);
```
So, for the C* case, if a keyspace is dropped then the whole range will be
removed, which will trigger that state check. I tested this via
```
@Test
void removeRanges()
{
TopologyManager service = new TopologyManager(SUPPLIER, ID);
addAndMarkSynced(service, topology(1, shard(range(0, 200), idList(1,
2, 3), idSet(1, 2))));
Topology t2 = topology(2, shard(range(0, 200), idList(1, 2, 3),
idSet(1, 2)));
Topology t3 = topology(3, shard(range(201, 400), idList(1, 2, 3),
idSet(1, 2)));
service.onTopologyUpdate(t2);
service.onTopologyUpdate(t3);
markTopologySynced(service, t2.epoch());
markTopologySynced(service, t3.epoch());
}
```
and see that the check is causing this to fail. The test will pass if the
(201,400] range is pending, but not when it's outside the "new" ranges set.
To hit this case, we need 2 pending topologies, where the older topology
completes before the `current` one does. We then use the `current` ranges. I
updated the check to show the ranges and we see its
```
Expected [(0,200]] to contain all ranges in [(201,400]]; but did not
```
this is at this line
```
markTopologySynced(service, t2.epoch());
```
so, logically, epoch 2 should "complete" epoch 1, but `markPrevSynced` is
being called on epoch 3
--
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]
