dcapwell commented on code in PR #6:
URL: https://github.com/apache/cassandra-accord/pull/6#discussion_r944031130
##########
accord-core/src/main/java/accord/local/Command.java:
##########
@@ -224,22 +224,19 @@ public boolean commit(Txn txn, Key homeKey, Key
progressKey, Timestamp executeAt
this.waitingOnCommit = new TreeMap<>();
this.waitingOnApply = new TreeMap<>();
- for (TxnId id : savedDeps().on(commandStore, executeAt))
- {
- Command command = commandStore.command(id);
+ savedDeps().forEachOn(commandStore, executeAt, txnId -> {
+ Command command = commandStore.command(txnId);
switch (command.status)
{
default:
throw new IllegalStateException();
case NotWitnessed:
- Txn depTxn = savedDeps().get(id);
- command.txn(depTxn);
case PreAccepted:
case Accepted:
case AcceptedInvalidate:
// we don't know when these dependencies will execute, and
cannot execute until we do
- waitingOnCommit.put(id, command);
command.addListener(this);
+ waitingOnCommit.put(txnId, command);
Review Comment:
is there a reason to move this after adding the listener? In both cases
they seem to update a map but shouldn't depend on the other?
##########
accord-core/src/main/java/accord/primitives/Keys.java:
##########
@@ -0,0 +1,449 @@
+package accord.primitives;
+
+import java.util.*;
+import java.util.function.IntFunction;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import accord.api.Key;
+import accord.utils.IndexedFold;
+import accord.utils.IndexedFoldIntersectToLong;
+import accord.utils.IndexedFoldToLong;
+import accord.utils.IndexedPredicate;
+import accord.utils.IndexedRangeFoldToLong;
+import accord.utils.SortedArrays;
+import org.apache.cassandra.utils.concurrent.Inline;
+
+@SuppressWarnings("rawtypes")
+// TODO: this should probably be a BTree
+// TODO: check that foldl call-sites are inlined and optimised by HotSpot
+public class Keys implements Iterable<Key>
+{
+ public static final Keys EMPTY = new Keys(new Key[0]);
+
+ final Key[] keys;
+
+ public Keys(SortedSet<? extends Key> keys)
+ {
+ this.keys = keys.toArray(Key[]::new);
+ }
+
+ public Keys(Collection<? extends Key> keys)
+ {
+ this.keys = keys.toArray(Key[]::new);
+ Arrays.sort(this.keys);
+ }
+
+ public Keys(Key[] keys)
+ {
+ this.keys = keys;
+ Arrays.sort(keys);
+ }
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ Keys keys1 = (Keys) o;
+ return Arrays.equals(keys, keys1.keys);
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return Arrays.hashCode(keys);
+ }
+
+ public int indexOf(Key key)
+ {
+ return Arrays.binarySearch(keys, key);
+ }
+
+ public boolean contains(Key key)
+ {
+ return indexOf(key) >= 0;
+ }
+
+ public Key get(int indexOf)
+ {
+ return keys[indexOf];
+ }
+
+ public boolean isEmpty()
+ {
+ return keys.length == 0;
+ }
+
+ public int size()
+ {
+ return keys.length;
+ }
+
+ public Keys select(int[] indexes)
+ {
+ Key[] selection = new Key[indexes.length];
+ for (int i = 0 ; i < indexes.length ; ++i)
+ selection[i] = keys[indexes[i]];
+ return new Keys(selection);
+ }
+
+ /**
+ * return true if this keys collection contains all keys found in the
given keys
+ */
+ public boolean containsAll(Keys that)
+ {
+ if (that.isEmpty())
+ return true;
+
+ return foldlIntersect(that, (li, ri, k, p, v) -> v + 1, 0, 0, 0) ==
that.size();
+ }
+
+ public Keys union(Keys that)
+ {
+ return wrap(SortedArrays.linearUnion(keys, that.keys, Key[]::new),
that);
+ }
+
+ public Keys intersect(Keys that)
+ {
+ return wrap(SortedArrays.linearIntersection(keys, that.keys,
Key[]::new), that);
+ }
+
+ public Keys slice(KeyRanges ranges)
+ {
+ return wrap(SortedArrays.sliceWithOverlaps(keys, ranges.ranges,
Key[]::new, (k, r) -> -r.compareTo(k), KeyRange::compareTo));
+ }
+
+ public int search(int lowerBound, int upperBound, Object key,
Comparator<Object> comparator)
Review Comment:
tried switching to `AsymmetricComparator` but the fact we are dealing with
`Key` and `KeyRange<K extends Key>` requires `K`, hit a type mismatch =(
Would need `Keys` to be `Keys<K extends Key>` to resolve
##########
accord-core/src/main/java/accord/primitives/KeyRange.java:
##########
@@ -163,14 +163,22 @@ public String toString()
* Returns a negative integer, zero, or a positive integer as the provided
key is less than, contained by,
* or greater than this range.
*/
- public abstract int compareKey(K key);
+ public int compareKey(K key)
+ {
+ return -compareTo(key);
+ }
+
+ /**
+ * Returns a negative integer, zero, or a positive integer as the provided
key is greater than, contained by,
+ * or less than this range.
+ */
+ public abstract int compareTo(K key);
Review Comment:
`@Override`
##########
accord-core/src/main/java/accord/primitives/Keys.java:
##########
@@ -0,0 +1,449 @@
+package accord.primitives;
+
+import java.util.*;
+import java.util.function.IntFunction;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import accord.api.Key;
+import accord.utils.IndexedFold;
+import accord.utils.IndexedFoldIntersectToLong;
+import accord.utils.IndexedFoldToLong;
+import accord.utils.IndexedPredicate;
+import accord.utils.IndexedRangeFoldToLong;
+import accord.utils.SortedArrays;
+import org.apache.cassandra.utils.concurrent.Inline;
+
+@SuppressWarnings("rawtypes")
+// TODO: this should probably be a BTree
+// TODO: check that foldl call-sites are inlined and optimised by HotSpot
+public class Keys implements Iterable<Key>
+{
+ public static final Keys EMPTY = new Keys(new Key[0]);
+
+ final Key[] keys;
+
+ public Keys(SortedSet<? extends Key> keys)
+ {
+ this.keys = keys.toArray(Key[]::new);
+ }
+
+ public Keys(Collection<? extends Key> keys)
+ {
+ this.keys = keys.toArray(Key[]::new);
+ Arrays.sort(this.keys);
+ }
+
+ public Keys(Key[] keys)
Review Comment:
in my review branch I made this private so I could remove the `sort`, see
adb09e7f2ce1084d3fe448e1c03e3312313a957c
For now I only allowed internal methods to use it to limit abuse, external
classes go through `of`
##########
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:
yeah, it looks like that is the case, to still allow would need a
```
public long foldl(KeyRanges rs, FoldToLong<Key> fold, long param, long
initialValue, long terminalValue)
```
but that adds more code... so this is fine.
##########
accord-core/src/main/java/accord/primitives/Keys.java:
##########
@@ -0,0 +1,449 @@
+package accord.primitives;
+
+import java.util.*;
+import java.util.function.IntFunction;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import accord.api.Key;
+import accord.utils.IndexedFold;
+import accord.utils.IndexedFoldIntersectToLong;
+import accord.utils.IndexedFoldToLong;
+import accord.utils.IndexedPredicate;
+import accord.utils.IndexedRangeFoldToLong;
+import accord.utils.SortedArrays;
+import org.apache.cassandra.utils.concurrent.Inline;
+
+@SuppressWarnings("rawtypes")
+// TODO: this should probably be a BTree
+// TODO: check that foldl call-sites are inlined and optimised by HotSpot
+public class Keys implements Iterable<Key>
+{
+ public static final Keys EMPTY = new Keys(new Key[0]);
+
+ final Key[] keys;
+
+ public Keys(SortedSet<? extends Key> keys)
+ {
+ this.keys = keys.toArray(Key[]::new);
+ }
+
+ public Keys(Collection<? extends Key> keys)
+ {
+ this.keys = keys.toArray(Key[]::new);
+ Arrays.sort(this.keys);
+ }
+
+ public Keys(Key[] keys)
+ {
+ this.keys = keys;
+ Arrays.sort(keys);
+ }
+
+ @Override
+ public boolean equals(Object o)
+ {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ Keys keys1 = (Keys) o;
+ return Arrays.equals(keys, keys1.keys);
+ }
+
+ @Override
+ public int hashCode()
+ {
+ return Arrays.hashCode(keys);
+ }
+
+ public int indexOf(Key key)
+ {
+ return Arrays.binarySearch(keys, key);
+ }
+
+ public boolean contains(Key key)
+ {
+ return indexOf(key) >= 0;
+ }
+
+ public Key get(int indexOf)
+ {
+ return keys[indexOf];
+ }
+
+ public boolean isEmpty()
+ {
+ return keys.length == 0;
+ }
+
+ public int size()
+ {
+ return keys.length;
+ }
+
+ public Keys select(int[] indexes)
Review Comment:
no code touches this
##########
accord-core/src/main/java/accord/messages/PreAccept.java:
##########
@@ -147,17 +148,23 @@ public String toString()
}
}
- static Dependencies calculateDeps(CommandStore commandStore, TxnId txnId,
Txn txn, Timestamp executeAt)
+// static Dependencies calculateDeps(CommandStore commandStore, TxnId
txnId, Txn txn, Timestamp executeAt)
Review Comment:
in my review branch I drop this commented out 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]
