dcapwell commented on code in PR #3694:
URL: https://github.com/apache/cassandra/pull/3694#discussion_r1868284194
##########
test/unit/org/apache/cassandra/cql3/RandomSchemaV2Test.java:
##########
@@ -98,7 +99,7 @@ public void test()
if (mode == Mode.AccordEnabled)
{
// enable accord
- schemaChange(String.format("ALTER TABLE %s WITH " +
TransactionalMode.full.asCqlParam(), metadata));
+ schemaChange(String.format("ALTER TABLE %s WITH " +
TransactionalMode.full.asCqlParam() + " AND " +
TransactionalMigrationFromMode.full.asCqlParam(), metadata));
Review Comment:
im not against this change but why this change? I thought the migration
mode was controlled from nodetool and not alter? This class also doesn't do
CAS (yet) so migration isn't touched?
##########
src/java/org/apache/cassandra/service/accord/api/AccordRoutingKey.java:
##########
@@ -98,20 +100,29 @@ public static AccordRoutingKey of(Key key)
// final in part because we refer to its class directly in
AccordRoutableKey.compareTo
public static final class SentinelKey extends AccordRoutingKey
{
- private static final long EMPTY_SIZE = ObjectSizes.measure(new
SentinelKey(null, true));
+ private static final long EMPTY_SIZE = ObjectSizes.measure(new
SentinelKey(null, true, false));
- public final boolean isMin;
+ // Is this a min sentinel or a max sentinel
+ public final boolean isMinSentinel;
- public SentinelKey(TableId table, boolean isMin)
+ // Is this a minumum of a max or min sentinel
+ // Allows conversion of a token key to a range
+ public final boolean isMinMinSentinel;
Review Comment:
TODO: this name is hard for me... need to figure out a recommendation...
##########
test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java:
##########
@@ -379,6 +381,49 @@ private void testRangeRead(int pageSize) throws Exception
});
}
+ @Test
+ public void testRangeReadSingleToken() throws Throwable
+ {
+ test(cluster ->
+ {
+ // This single partition read happens to execute as a range
read (at least when this test was created)
+ // and that exposed a problem with single token range reads
+ ICoordinator node = cluster.coordinator(1);
+ cluster.schemaChange(withKeyspace("CREATE TABLE
%s.testRangeReadSingleToken (pk0 int, ck0 int, static0 int, regular0 int,
PRIMARY KEY (pk0, ck0)) WITH " + transactionalMode.asCqlParam() + " AND
CLUSTERING ORDER BY (ck0 ASC);"));
+ cluster.schemaChange(withKeyspace("CREATE INDEX ck0_sai_idx
ON %s.testRangeReadSingleToken (ck0) USING 'sai';"));
+ node.executeWithResult(withKeyspace("INSERT INTO
%s.testRangeReadSingleToken (pk0, ck0, static0, regular0) VALUES (?, ?, ?,
?)"), QUORUM, 42, 43, 44, 45);
+ assertThat(node.executeWithResult(withKeyspace("SELECT pk0,
ck0, static0, regular0 FROM %s.testRangeReadSingleToken WHERE pk0 = ? AND ck0 =
? AND static0 <= ? AND regular0 >= ? ALLOW FILTERING;"), ConsistencyLevel.ALL,
42, 43, 44, 45))
+ .isEqualTo(42, 43, 44, 45);
+
+ // This one is a little more explicit about trying to force a
range read of a single token
+ cluster.schemaChange(withKeyspace("CREATE TABLE
%s.testRangeReadSingleToken2 (pk blob primary key) WITH " +
TransactionalMode.full.asCqlParam()));
+ long token = Long.MIN_VALUE;
+ ByteBuffer keyForToken =
Murmur3Partitioner.LongToken.keyForToken(token);
+ node.executeWithResult(withKeyspace("INSERT INTO
%s.testRangeReadSingleToken2 (pk) VALUES (?)"), QUORUM, keyForToken);
+ assertThat(node.executeWithResult(withKeyspace("SELECT * FROM
%s.testRangeReadSingleToken2 WHERE token(pk) >= token(?) AND token(pk) <=
token(?)"), QUORUM, Murmur3Partitioner.LongToken.keyForToken(token),
keyForToken))
+ .isEqualTo(keyForToken);
+ });
+ }
+
+ @Test
+ public void testRangeReadRightMin() throws Throwable
+ {
+ test(cluster ->
+ {
+ ICoordinator node = cluster.coordinator(1);
+ cluster.schemaChange(withKeyspace("CREATE TABLE
%s.testRangeReadRightMin (pk blob primary key) WITH " +
TransactionalMode.full.asCqlParam()));
+ long token = Long.MIN_VALUE;
+ ByteBuffer keyForToken =
Murmur3Partitioner.LongToken.keyForToken(token);
+ node.executeWithResult(withKeyspace("INSERT INTO
%s.testRangeReadRightMin (pk) VALUES (?)"), QUORUM, keyForToken);
+ assertThat(node.executeWithResult(withKeyspace("SELECT * FROM
%s.testRangeReadRightMin WHERE token(pk) >= token(?)"), QUORUM, keyForToken))
+ .isEqualTo(keyForToken);
+ assertThat(node.executeWithResult(withKeyspace("SELECT * FROM
%s.testRangeReadRightMin WHERE token(pk) > token(?) AND token(pk) < token(?)"),
QUORUM, Murmur3Partitioner.LongToken.keyForToken(0), keyForToken))
+ .isEmpty();
+ assertThat(node.executeWithResult(withKeyspace("SELECT * FROM
%s.testRangeReadRightMin WHERE token(pk) > token(?) AND token(pk) <=
token(?)"), QUORUM, Murmur3Partitioner.LongToken.keyForToken(0), keyForToken))
Review Comment:
can we include `token(pk) between token(?) and token(?)` as well? it would
produce a different range than the one this test covers and would improve
coverage
--
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]
