dcapwell commented on code in PR #6:
URL: https://github.com/apache/cassandra-accord/pull/6#discussion_r937209610
##########
accord-core/src/main/java/accord/local/CommandStores.java:
##########
@@ -208,10 +207,10 @@ KeyRanges currentRanges()
static long keyIndex(Key key, long numShards)
{
- return Integer.toUnsignedLong(key.keyHash()) % numShards;
+ return Integer.toUnsignedLong(key.routingHash()) % numShards;
}
- private static long addKeyIndex(Key key, long numShards, long
accumulate)
+ private static long addKeyIndex(int i, Key key, long numShards, long
accumulate)
Review Comment:
`i` is dead code, is this still needed?
##########
accord-core/src/main/java/accord/messages/BeginRecovery.java:
##########
@@ -78,17 +78,19 @@ public void process(Node node, Id replyToNode, ReplyContext
replyContext)
// committed txns with an earlier txnid and have our txnid as
a dependency
earlierCommittedWitness = committedStartedBefore(instance,
txnId, txn.keys)
.filter(c ->
c.savedDeps().contains(txnId))
- .collect(Dependencies::new,
Dependencies::add, Dependencies::addAll);
+ .collect(() ->
Deps.builder(txn.keys), Deps.Builder::add, (a, b) -> { throw new
IllegalStateException(); })
Review Comment:
same logic as line 89, maybe best if we add a `Deps.collector(txn.keys)`
that looks like the following
```
public static Collector<Command, Builder, Builder> collector(Keys keys)
{
return Collector.of(() -> Deps.builder(keys), Deps.Builder::add, (a, b)
-> { throw new IllegalStateException(); });
}
```
##########
accord-core/src/main/java/accord/utils/Timestamped.java:
##########
@@ -17,4 +17,10 @@ public static <T> Timestamped<T> merge(Timestamped<T> a,
Timestamped<T> b)
{
return a.timestamp.compareTo(b.timestamp) >= 0 ? a : b;
}
+
+ @Override
+ public String toString()
Review Comment:
given that this also has timestamp would it make sense to add to string as
well?
##########
accord-core/src/main/java/accord/txn/Txn.java:
##########
@@ -4,12 +4,15 @@
import accord.api.*;
import accord.local.*;
+import accord.primitives.Keys;
+import accord.primitives.Timestamp;
public class Txn
{
- enum Kind { READ, WRITE, RECONFIGURE }
Review Comment:
removing `RECONFIGURE` for now or is this now handled differently? Want to
make sure I don't miss.
##########
accord-core/src/main/java/accord/api/Key.java:
##########
@@ -3,10 +3,7 @@
/**
* A routing key for determining which shards are involved in a transaction
*/
-public interface Key<K extends Key<K>> extends Comparable<K>
+public interface Key<K extends Key<K>> extends RoutingKey<K>
{
- /**
- * Returns a hash code of a key to support accord internal sharding. Hash
values for equal keys must be equal.
- */
- int keyHash();
+ Key toRoutingKey();
Review Comment:
> toRoutingKey appears to be dead in this patch set yes, I'll remove it.
just reposting so it doesn't get lost, seems after the last update It
doesn't show up while reading the code.
--
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]
