iamaleksey commented on code in PR #2073:
URL: https://github.com/apache/cassandra/pull/2073#discussion_r1067180852
##########
src/java/org/apache/cassandra/service/accord/AccordService.java:
##########
@@ -31,6 +31,8 @@
import accord.impl.SimpleProgressLog;
import accord.impl.SizeOfIntersectionSorter;
import accord.local.Node;
+import accord.local.ShardDistributor;
Review Comment:
Dead import
##########
src/java/org/apache/cassandra/service/accord/AccordService.java:
##########
@@ -41,6 +43,8 @@
import org.apache.cassandra.exceptions.WriteTimeoutException;
import org.apache.cassandra.net.IVerbHandler;
import org.apache.cassandra.service.accord.api.AccordAgent;
+import org.apache.cassandra.service.accord.api.AccordRoutingKey;
Review Comment:
Dead import
##########
src/java/org/apache/cassandra/db/marshal/ByteArrayAccessor.java:
##########
@@ -107,6 +108,7 @@ public byte[] read(DataInputPlus in, int length) throws
IOException
@Override
public byte[] slice(byte[] input, int offset, int length)
{
+ Invariants.checkArgument(offset + length <= input.length);
Review Comment:
Maybe more appropriate and consistent to use `Preconditions.checkArgument()`
in a super-generic utility class like this?
##########
src/java/org/apache/cassandra/service/accord/TokenRange.java:
##########
@@ -36,9 +36,9 @@ public TokenRange(AccordRoutingKey start, AccordRoutingKey
end)
super(start, end);
}
- public static TokenRange fullRange(TableId tableId)
+ public static TokenRange fullRange(String keyspace, TableId tableId)
Review Comment:
`TableId` arg now unused
##########
test/unit/org/apache/cassandra/service/accord/api/AccordKeyTest.java:
##########
@@ -91,10 +91,22 @@ public void tableComparisonTest()
Assert.assertTrue(TABLE1.compareTo(TABLE2) < 0);
DecoratedKey dk1 =
partitioner(TABLE1).decorateKey(ByteBufferUtil.bytes(5));
- PartitionKey pk1 = new PartitionKey(TABLE1, dk1);
+ PartitionKey pk1 = new PartitionKey("", TABLE1, dk1);
DecoratedKey dk2 =
partitioner(TABLE2).decorateKey(ByteBufferUtil.bytes(5));
- PartitionKey pk2 = new PartitionKey(TABLE2, dk2);
+ PartitionKey pk2 = new PartitionKey("", TABLE2, dk2);
+
+ Assert.assertTrue(pk1.compareTo(pk2) == 0);
Review Comment:
Nope
##########
src/java/org/apache/cassandra/service/accord/async/AsyncOperation.java:
##########
@@ -65,6 +66,7 @@
private State state = State.INITIALIZED;
private final AccordCommandStore commandStore;
+ private SafeAccordCommandStore safeStore;
Review Comment:
I don't think we need this field? Should be fine with just a local variable
under `case LOADING`? Am I missing something important?
##########
src/java/org/apache/cassandra/dht/IPartitioner.java:
##########
@@ -24,6 +24,7 @@
import java.util.Optional;
import java.util.Random;
+import accord.local.ShardDistributor;
Review Comment:
Redundant import
##########
test/unit/org/apache/cassandra/service/accord/api/AccordKeyTest.java:
##########
@@ -59,26 +59,26 @@ public static IPartitioner partitioner(TableId tableId)
public void partitionKeyTest()
{
DecoratedKey dk =
partitioner(TABLE1).decorateKey(ByteBufferUtil.bytes(5));
- PartitionKey pk = new PartitionKey(TABLE1, dk);
+ PartitionKey pk = new PartitionKey("ks", TABLE1, dk);
SerializerTestUtils.assertSerializerIOEquality(pk,
PartitionKey.serializer);
}
@Test
public void tokenKeyTest()
{
DecoratedKey dk =
partitioner(TABLE1).decorateKey(ByteBufferUtil.bytes(5));
- TokenKey pk = new TokenKey(TABLE1, dk.getToken());
+ TokenKey pk = new TokenKey("", dk.getToken());
SerializerTestUtils.assertSerializerIOEquality(pk,
TokenKey.serializer);
}
@Test
public void comparisonTest()
{
DecoratedKey dk =
partitioner(TABLE1).decorateKey(ByteBufferUtil.bytes(5));
- PartitionKey pk = new PartitionKey(TABLE1, dk);
- TokenKey tk = new TokenKey(TABLE1, dk.getToken());
- TokenKey tkLow = new TokenKey(TABLE1,
dk.getToken().decreaseSlightly());
- TokenKey tkHigh = new TokenKey(TABLE1,
dk.getToken().increaseSlightly());
+ PartitionKey pk = new PartitionKey("", TABLE1, dk);
+ TokenKey tk = new TokenKey("", dk.getToken());
+ TokenKey tkLow = new TokenKey("", dk.getToken().decreaseSlightly());
+ TokenKey tkHigh = new TokenKey("", dk.getToken().increaseSlightly());
Assert.assertTrue(tk.compareTo(pk) == 0);
Review Comment:
No longer yep
##########
test/unit/org/apache/cassandra/service/accord/api/AccordKeyTest.java:
##########
@@ -59,26 +59,26 @@ public static IPartitioner partitioner(TableId tableId)
public void partitionKeyTest()
{
DecoratedKey dk =
partitioner(TABLE1).decorateKey(ByteBufferUtil.bytes(5));
- PartitionKey pk = new PartitionKey(TABLE1, dk);
+ PartitionKey pk = new PartitionKey("ks", TABLE1, dk);
Review Comment:
Completely insignificant, but is there a reason to use `"ks"` here (that
exists) vs `""` elsewhere below?
##########
src/java/org/apache/cassandra/db/rows/AbstractCell.java:
##########
@@ -224,7 +224,7 @@ public String toString()
if (isTombstone())
return String.format("[%s=<tombstone> %s]", column().name,
livenessInfoString());
else
- return String.format("[%s=%s %s]", column().name,
safeToString(type), livenessInfoString());
+ return String.format("[%s=%s %s]", column().name,
safeToString(column.type), livenessInfoString());
Review Comment:
Why?
--
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]
