aweisberg commented on code in PR #46:
URL: https://github.com/apache/cassandra-accord/pull/46#discussion_r1200906647
##########
accord-core/src/main/java/accord/primitives/Ranges.java:
##########
@@ -227,4 +230,27 @@ else if (count == result.length)
return construct(cachedRanges.completeAndDiscard(result, count));
}
+ public Partition partition(Predicate<Range> test)
+ {
+ List<Range> left = new ArrayList<>();
+ List<Range> right = new ArrayList<>();
+ for (Range range : this)
+ (test.test(range) ? left : right).add(range);
Review Comment:
Left and right is odd terminology when this is really filtering into
true/false buckets for the predicate.
##########
accord-core/src/main/java/accord/local/CommandStores.java:
##########
@@ -371,9 +376,12 @@ private synchronized TopologyUpdate updateTopology(Node
node, Snapshot prev, Top
RangesForEpochHolder rangesHolder = new RangesForEpochHolder();
ShardHolder shardHolder = new
ShardHolder(supplier.create(nextId++, rangesHolder), rangesHolder);
rangesHolder.current = new RangesForEpoch(epoch, add,
shardHolder.store);
- // the first epoch we assume is either empty, or correctly
initialised by whatever system is migrating
- if (epoch == 1) bootstrapUpdates.add(() ->
shardHolder.store.initialise(epoch, add));
- else bootstrapUpdates.add(shardHolder.store.bootstrapper(node,
add, newLocalTopology.epoch()));
+
+ Ranges.Partition partition = add.partition(range ->
shouldBootstrap(node, prev.local, newLocalTopology, range));
Review Comment:
I don't understand this. The partition will put all the ranges on the left
or right based on `newLocalTopology.epoch` so why partition at all?
##########
accord-core/src/main/java/accord/primitives/TxnId.java:
##########
@@ -35,6 +35,13 @@ public static TxnId fromBits(long msb, long lsb, Id node)
return new TxnId(msb, lsb, node);
}
+ public static TxnId fromTimestamp(Timestamp t)
Review Comment:
This bypasses the `TxnId` vs `Timestamp` distinction for test code and makes
it seem like they are the same thing. Maybe I am being to pedantic.
Since the only usage is `fromString` maybe add `TxnId.fromString` instead
and have it create the `TxnId` from the `Timestamp`?
##########
accord-core/src/main/java/accord/topology/Topology.java:
##########
@@ -258,16 +260,36 @@ private <P1> int[] subsetFor(Unseekables<?, ?> select,
IndexedPredicate<P1> pred
if (abi < 0)
{
if (ailim < as.size())
- throw new IllegalArgumentException("Range not
found for " + as.get(ailim));
+ {
+ switch (onUnknown)
+ {
+ case REJECT: throw new
IllegalArgumentException("Range not found for " + as.get(ailim));
+ case IGNORE:
+ break;
+ default:
+ throw new
IllegalArgumentException("Unknown option: " + onUnknown);
+ }
+ }
break;
}
ai = (int)abi;
+ boolean skip = false;
if (ailim < ai)
- throw new IllegalArgumentException("Range not found
for " + as.get(ailim));
+ {
+ switch (onUnknown)
+ {
+ default:
+ throw new IllegalArgumentException("Unknown
option: " + onUnknown);
+ case REJECT: throw new
IllegalArgumentException("Range not found for " + as.get(ailim));
+ case IGNORE:
+ bi = (int)(abi >>> 32);
Review Comment:
Unused
##########
accord-core/src/main/java/accord/topology/TopologyManager.java:
##########
@@ -230,16 +230,20 @@ public synchronized void onTopologyUpdate(Topology
topology)
{
Epochs current = epochs;
- checkArgument(topology.epoch == current.nextEpoch());
+ checkArgument(topology.epoch == current.nextEpoch(), "Expected
topology update %d to be %d", topology.epoch, current.nextEpoch());
EpochState[] nextEpochs = new EpochState[current.epochs.length + 1];
List<Set<Id>> pendingSync = new
ArrayList<>(current.pendingSyncComplete);
Set<Id> alreadySyncd = Collections.emptySet();
if (!pendingSync.isEmpty())
{
- EpochState currentEpoch = current.epochs[0];
- if (current.epochs[0].syncComplete())
- currentEpoch.markPrevSynced();
- alreadySyncd = pendingSync.remove(0);
+ // if empty, then notified about an epoch from a peer before first
epoch seen
+ if (current.epochs.length != 0)
Review Comment:
Burn test fix?
##########
accord-core/src/main/java/accord/topology/Topology.java:
##########
@@ -236,7 +236,9 @@ private Topology forSubset(int[] newSubset, Collection<Id>
nodes)
return new Topology(epoch, shards, ranges, nodeLookup, rangeSubset,
newSubset);
}
- private <P1> int[] subsetFor(Unseekables<?, ?> select,
IndexedPredicate<P1> predicate, P1 param)
+ private enum OnUnknown { REJECT, IGNORE }
Review Comment:
It kind of makes sense as a work around for the case you are describing,
where is this code that tries to compare different epochs that fails?
It seems like this change is opting everything into the `IGNORE` behavior?
--
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]
