iamaleksey commented on code in PR #20:
URL: https://github.com/apache/cassandra-accord/pull/20#discussion_r1064924655
##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -38,66 +36,25 @@
public interface Factory
{
CommandStore create(int id,
- int generation,
- int shardIndex,
- int numShards,
NodeTimeService time,
Agent agent,
DataStore store,
ProgressLog.Factory progressLogFactory,
- RangesForEpoch rangesForEpoch);
- }
-
- public interface RangesForEpoch
- {
- Ranges at(long epoch);
- Ranges between(long fromInclusive, long toInclusive);
- Ranges since(long epoch);
- boolean owns(long epoch, RoutingKey key);
+ RangesForEpochHolder rangesForEpoch);
}
private final int id; // unique id
- private final int generation;
- private final int shardIndex;
- private final int numShards;
- public CommandStore(int id,
- int generation,
- int shardIndex,
- int numShards)
+ public CommandStore(int id)
{
this.id = id;
- this.generation = generation;
- this.shardIndex = checkArgument(shardIndex, shardIndex < numShards);
- this.numShards = numShards;
}
public int id()
{
return id;
}
Review Comment:
Not even a nit: redundant newline?
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -205,7 +213,9 @@ public void forCommittedInEpoch(Ranges ranges, long epoch,
Consumer<Command> con
for (InMemoryCommandsForKey commands : rangeCommands)
{
Collection<Command> committed =
commands.committedByExecuteAt()
- .between(minTimestamp, maxTimestamp).map(cmd ->
(Command) cmd).collect(Collectors.toList());
+ .between(minTimestamp, maxTimestamp)
+ .collect(Collectors.toList());
Review Comment:
No need to `.collect()` here, can `.forEach()` directly.
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStores.java:
##########
@@ -34,9 +34,9 @@
{
public static class Synchronized extends SyncCommandStores
{
- public Synchronized(int num, Node node, Agent agent, DataStore store,
ProgressLog.Factory progressLogFactory)
+ public Synchronized(NodeTimeService time, Agent agent, DataStore
store, ShardDistributor shardDistributor, ProgressLog.Factory
progressLogFactory)
Review Comment:
`Node` import now unused.
##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -19,17 +19,15 @@
package accord.local;
import accord.api.*;
-import accord.local.CommandStores.ShardedRanges;
import accord.api.ProgressLog;
-import accord.primitives.*;
+import accord.local.CommandStores.RangesForEpoch;
Review Comment:
Import is no longer needed.
##########
accord-core/src/test/java/accord/impl/IntHashKey.java:
##########
@@ -48,14 +111,9 @@ private Key(int key, int hash)
public static final class Hash extends IntHashKey implements RoutingKey
{
- private Hash(int key)
+ public Hash(int hash)
{
- super(key);
- }
-
- private Hash(int key, int hash)
Review Comment:
`Key(int key, int hash)` constructor is also dead code now.
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -60,19 +61,20 @@ public static abstract class State implements
SafeCommandStore
private final Agent agent;
private final DataStore store;
private final ProgressLog progressLog;
- private final RangesForEpoch rangesForEpoch;
+ private final RangesForEpochHolder rangesForEpochHolder;
+ private RangesForEpoch rangesForEpoch;
Review Comment:
Throw in a != null invariant check into `ranges()` accessor? It's a bit
silly, and seems to be only accessed in safe store context, so no NPEs now, but
somehow bothers me slightly.
##########
accord-core/src/main/java/accord/local/CommandStores.java:
##########
@@ -269,70 +254,36 @@ private Snapshot updateTopology(Snapshot prev, Topology
newTopology)
Topology newLocalTopology =
newTopology.forNode(supplier.time.id()).trim();
Ranges added =
newLocalTopology.ranges().difference(prev.local.ranges());
Ranges subtracted =
prev.local.ranges().difference(newLocalTopology.ranges());
-// for (ShardedRanges range : stores.ranges)
-// {
-// // FIXME: remove this (and the corresponding check in
TopologyRandomizer) once lower bounds are implemented.
-// // In the meantime, the logic needed to support acquiring
ranges that we previously replicated is pretty
-// // convoluted without the ability to jettison epochs.
-// Invariants.checkState(!range.ranges.intersects(added));
-// }
if (added.isEmpty() && subtracted.isEmpty())
- return new Snapshot(prev.ranges, newTopology, newLocalTopology);
+ return new Snapshot(prev.shards, newLocalTopology, newTopology);
- ShardedRanges[] result = new ShardedRanges[prev.ranges.length +
(added.isEmpty() ? 0 : 1)];
+ List<ShardHolder> result = new ArrayList<>(prev.shards.length +
added.size());
if (subtracted.isEmpty())
{
- int newGeneration = prev.ranges.length;
- System.arraycopy(prev.ranges, 0, result, 0, newGeneration);
- result[newGeneration] =
supplier.createShardedRanges(newGeneration, epoch, added,
rangesForEpochFunction(newGeneration));
+ Collections.addAll(result, prev.shards);
}
else
{
- int i = 0;
- while (i < prev.ranges.length)
+ for (ShardHolder shard : prev.shards)
{
- ShardedRanges ranges = prev.ranges[i];
- if (ranges.currentRanges().intersects(subtracted))
- ranges = ranges.withRanges(newTopology.epoch(),
ranges.currentRanges().difference(subtracted));
- result[i++] = ranges;
+ if (subtracted.intersects(shard.ranges().currentRanges()))
+ shard.ranges.current =
shard.ranges().withRanges(newTopology.epoch(),
shard.ranges().currentRanges().difference(subtracted));
+ result.add(shard);
}
- if (i < result.length)
- result[i] = supplier.createShardedRanges(i, epoch, added,
rangesForEpochFunction(i));
}
- return new Snapshot(result, newTopology, newLocalTopology);
- }
-
- private RangesForEpoch rangesForEpochFunction(int generation)
- {
- return new RangesForEpoch()
+ if (!added.isEmpty())
Review Comment:
As discussed off-GH, could use a TODO to acknowledge the temporary state of
this logic and intended behaviour (no growth in shard count).
--
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]
