You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2025/12/09 19:25:08 UTC
[PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
dcapwell opened a new pull request, #4518:
URL: https://github.com/apache/cassandra/pull/4518
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2607590625
##########
src/java/org/apache/cassandra/batchlog/BatchlogManager.java:
##########
@@ -451,9 +453,12 @@ public void finish(Set<UUID> hintedNodes)
{
if (accordResult != null)
{
- TxnResult.Kind kind = accordResult.awaitAndGet().kind();
+ TxnResult result = accordResult.awaitAndGet();
+ TxnResult.Kind kind = result.kind();
if (kind == retry_new_protocol)
throw new RetryOnDifferentSystemException();
+ if (result.kind() == txn_data)
+ ((TxnData) result).checkAndThrowValidationException();
Review Comment:
honestly didn't look closely, I focused on hitting each code that touched this coordinate logic outside of tools.
Without accord batch is given the mutations, which means we would reject before hitting batchlog I think; now its *after* we give... ill look closer and see if i can hit this code path in tests
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2611539096
##########
src/java/org/apache/cassandra/batchlog/BatchlogManager.java:
##########
@@ -451,9 +453,12 @@ public void finish(Set<UUID> hintedNodes)
{
if (accordResult != null)
{
- TxnResult.Kind kind = accordResult.awaitAndGet().kind();
+ TxnResult result = accordResult.awaitAndGet();
+ TxnResult.Kind kind = result.kind();
if (kind == retry_new_protocol)
throw new RetryOnDifferentSystemException();
+ if (result.kind() == txn_data)
+ ((TxnData) result).checkAndThrowValidationException();
Review Comment:
thought about it more and i don't think this code path is actually possible. We send `Mutation` to the batch so have already passed validation. We also know its not the accord code path as `retry_new_protocol` tells batch we need to reprocess as accord...
Ill remove
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612006574
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnReferenceOperationTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Set;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import accord.utils.Gen;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.FieldIdentifier;
+import org.apache.cassandra.cql3.Operation;
+import org.apache.cassandra.cql3.terms.Constants;
+import org.apache.cassandra.cql3.terms.UserTypes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.accord.serializers.TableMetadatas;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.Generators;
+import org.reflections.Reflections;
+import org.reflections.scanners.Scanners;
+import org.reflections.util.ConfigurationBuilder;
+
+import static accord.utils.Property.qt;
+
+public class TxnReferenceOperationTest
+{
+ private static final String KS = "ks";
+ private static final TxnData EMPTY = new TxnData();
+
+ @Test
+ public void coverage()
+ {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .forPackage("org.apache.cassandra")
+ .setScanners(Scanners.SubTypes)
+ .setExpandSuperTypes(true)
+ .setParallel(true));
+ var subTypes = reflections.getSubTypesOf(Operation.class);
+ var knownTypes = TxnReferenceOperation.initOperationKindMap().keySet();
+ // these types do not have a way to define a reference, as they are column level:
+ Set<Class<? extends Operation>> safeToExclude = ImmutableSet.of(Constants.Deleter.class, // DELETE foo
+ UserTypes.DeleterByField.class // DELETE foo.bar
+ );
+
+ StringBuilder sb = null;
+ for (var klass : com.google.common.collect.Sets.difference(subTypes, com.google.common.collect.Sets.union(knownTypes, safeToExclude)))
+ {
+ if (sb == null)
+ sb = new StringBuilder();
+ sb.append(klass.getCanonicalName()).append('\n');
+ }
+ if (sb != null)
+ throw new AssertionError(sb.toString());
+ }
+
+ @Test
+ public void serde()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().withExamples(10_000).forAll(gen()).check(txnOp -> {
+ Serializers.testSerde(output, TxnReferenceOperation.serializer, txnOp, TableMetadatas.of(txnOp.table));
+
+ txnOp.toOperation(EMPTY);
+ });
+ }
+
+ private enum Group
+ {
+ Setter, SetterByIndex, SetterByKey, SetterByField,
+ Adder, Subtracter,
+ Appender, Putter, Discarder, Prepender,
+ }
+
+ private static Gen<TxnReferenceValue> valueGen(AbstractType<?> type)
+ {
+ return Generators.toGen(AbstractTypeGenerators.getTypeSupport(type)
+ .bytesGen())
+ .map(TxnReferenceValue.Constant::new);
+ }
+
+ private static Gen<TxnReferenceOperation> gen()
Review Comment:
nit: Should this go in `AccordGenerators`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604052894
##########
src/java/org/apache/cassandra/exceptions/RequestValidationException.java:
##########
@@ -28,4 +28,12 @@ protected RequestValidationException(ExceptionCode code, String msg, Throwable e
{
super(code, msg, e);
}
+
+ public static class UnsafeException extends RequestValidationException
Review Comment:
this is super ugly, but lets us handle every type... i can also make the base class not abstract...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2608317296
##########
src/java/org/apache/cassandra/batchlog/BatchlogManager.java:
##########
@@ -451,9 +453,12 @@ public void finish(Set<UUID> hintedNodes)
{
if (accordResult != null)
{
- TxnResult.Kind kind = accordResult.awaitAndGet().kind();
+ TxnResult result = accordResult.awaitAndGet();
+ TxnResult.Kind kind = result.kind();
if (kind == retry_new_protocol)
throw new RetryOnDifferentSystemException();
+ if (result.kind() == txn_data)
+ ((TxnData) result).checkAndThrowValidationException();
Review Comment:
moved to `Result` based but accord isn't consistent and `execute` *happens after* `result` in the common case; if we can flip this order than this result logic isn't so hacky
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612012378
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnReferenceOperationTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Set;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import accord.utils.Gen;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.FieldIdentifier;
+import org.apache.cassandra.cql3.Operation;
+import org.apache.cassandra.cql3.terms.Constants;
+import org.apache.cassandra.cql3.terms.UserTypes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.accord.serializers.TableMetadatas;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.Generators;
+import org.reflections.Reflections;
+import org.reflections.scanners.Scanners;
+import org.reflections.util.ConfigurationBuilder;
+
+import static accord.utils.Property.qt;
+
+public class TxnReferenceOperationTest
+{
+ private static final String KS = "ks";
+ private static final TxnData EMPTY = new TxnData();
+
+ @Test
+ public void coverage()
+ {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .forPackage("org.apache.cassandra")
+ .setScanners(Scanners.SubTypes)
+ .setExpandSuperTypes(true)
+ .setParallel(true));
+ var subTypes = reflections.getSubTypesOf(Operation.class);
+ var knownTypes = TxnReferenceOperation.initOperationKindMap().keySet();
+ // these types do not have a way to define a reference, as they are column level:
+ Set<Class<? extends Operation>> safeToExclude = ImmutableSet.of(Constants.Deleter.class, // DELETE foo
+ UserTypes.DeleterByField.class // DELETE foo.bar
+ );
+
+ StringBuilder sb = null;
+ for (var klass : com.google.common.collect.Sets.difference(subTypes, com.google.common.collect.Sets.union(knownTypes, safeToExclude)))
+ {
+ if (sb == null)
+ sb = new StringBuilder();
+ sb.append(klass.getCanonicalName()).append('\n');
+ }
+ if (sb != null)
+ throw new AssertionError(sb.toString());
+ }
+
+ @Test
+ public void serde()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().withExamples(10_000).forAll(gen()).check(txnOp -> {
+ Serializers.testSerde(output, TxnReferenceOperation.serializer, txnOp, TableMetadatas.of(txnOp.table));
+
+ txnOp.toOperation(EMPTY);
+ });
+ }
+
+ private enum Group
+ {
+ Setter, SetterByIndex, SetterByKey, SetterByField,
+ Adder, Subtracter,
+ Appender, Putter, Discarder, Prepender,
+ }
+
+ private static Gen<TxnReferenceValue> valueGen(AbstractType<?> type)
+ {
+ return Generators.toGen(AbstractTypeGenerators.getTypeSupport(type)
+ .bytesGen())
+ .map(TxnReferenceValue.Constant::new);
+ }
+
+ private static Gen<TxnReferenceOperation> gen()
+ {
+ return rs -> {
+ TxnReferenceOperation.Kind kind;
+ ColumnMetadata receiver;
+ TableMetadata table;
+ @Nullable ByteBuffer key = null;
Review Comment:
```suggestion
@Nullable ByteBuffer keyOrIndex = null;
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612232251
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnReferenceOperationTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Set;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import accord.utils.Gen;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.FieldIdentifier;
+import org.apache.cassandra.cql3.Operation;
+import org.apache.cassandra.cql3.terms.Constants;
+import org.apache.cassandra.cql3.terms.UserTypes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.accord.serializers.TableMetadatas;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.Generators;
+import org.reflections.Reflections;
+import org.reflections.scanners.Scanners;
+import org.reflections.util.ConfigurationBuilder;
+
+import static accord.utils.Property.qt;
+
+public class TxnReferenceOperationTest
+{
+ private static final String KS = "ks";
+ private static final TxnData EMPTY = new TxnData();
+
+ @Test
+ public void coverage()
+ {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .forPackage("org.apache.cassandra")
+ .setScanners(Scanners.SubTypes)
+ .setExpandSuperTypes(true)
+ .setParallel(true));
+ var subTypes = reflections.getSubTypesOf(Operation.class);
+ var knownTypes = TxnReferenceOperation.initOperationKindMap().keySet();
+ // these types do not have a way to define a reference, as they are column level:
+ Set<Class<? extends Operation>> safeToExclude = ImmutableSet.of(Constants.Deleter.class, // DELETE foo
+ UserTypes.DeleterByField.class // DELETE foo.bar
+ );
+
+ StringBuilder sb = null;
+ for (var klass : com.google.common.collect.Sets.difference(subTypes, com.google.common.collect.Sets.union(knownTypes, safeToExclude)))
Review Comment:
we have `org.apache.cassandra.cql3.terms.Sets` imported =)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2606125145
##########
src/java/org/apache/cassandra/service/accord/txn/TxnDataValue.java:
##########
@@ -41,22 +43,27 @@ interface TxnDataValueSerializer<T extends TxnDataValue> extends IVersionedSeria
enum Kind
{
key(0),
- range(1);
+ range(1),
+ message(2);
Review Comment:
Why are we embdedding it in `TxnData` rather than making a new kind of `TxnResult`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2607582859
##########
src/java/org/apache/cassandra/service/accord/txn/TxnDataValue.java:
##########
@@ -41,22 +43,27 @@ interface TxnDataValueSerializer<T extends TxnDataValue> extends IVersionedSeria
enum Kind
{
key(0),
- range(1);
+ range(1),
+ message(2);
Review Comment:
the caller that adds this metadata is in `org.apache.cassandra.service.accord.txn.TxnUpdate#apply` which is *given* a `Data data`, I don't think I can override it here?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612247144
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnReferenceOperationTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Set;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import accord.utils.Gen;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.FieldIdentifier;
+import org.apache.cassandra.cql3.Operation;
+import org.apache.cassandra.cql3.terms.Constants;
+import org.apache.cassandra.cql3.terms.UserTypes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.accord.serializers.TableMetadatas;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.Generators;
+import org.reflections.Reflections;
+import org.reflections.scanners.Scanners;
+import org.reflections.util.ConfigurationBuilder;
+
+import static accord.utils.Property.qt;
+
+public class TxnReferenceOperationTest
+{
+ private static final String KS = "ks";
+ private static final TxnData EMPTY = new TxnData();
+
+ @Test
+ public void coverage()
+ {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .forPackage("org.apache.cassandra")
+ .setScanners(Scanners.SubTypes)
+ .setExpandSuperTypes(true)
+ .setParallel(true));
+ var subTypes = reflections.getSubTypesOf(Operation.class);
+ var knownTypes = TxnReferenceOperation.initOperationKindMap().keySet();
+ // these types do not have a way to define a reference, as they are column level:
+ Set<Class<? extends Operation>> safeToExclude = ImmutableSet.of(Constants.Deleter.class, // DELETE foo
+ UserTypes.DeleterByField.class // DELETE foo.bar
+ );
+
+ StringBuilder sb = null;
+ for (var klass : com.google.common.collect.Sets.difference(subTypes, com.google.common.collect.Sets.union(knownTypes, safeToExclude)))
+ {
+ if (sb == null)
+ sb = new StringBuilder();
+ sb.append(klass.getCanonicalName()).append('\n');
+ }
+ if (sb != null)
+ throw new AssertionError(sb.toString());
+ }
+
+ @Test
+ public void serde()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().withExamples(10_000).forAll(gen()).check(txnOp -> {
+ Serializers.testSerde(output, TxnReferenceOperation.serializer, txnOp, TableMetadatas.of(txnOp.table));
+
+ txnOp.toOperation(EMPTY);
+ });
+ }
+
+ private enum Group
+ {
+ Setter, SetterByIndex, SetterByKey, SetterByField,
+ Adder, Subtracter,
+ Appender, Putter, Discarder, Prepender,
+ }
+
+ private static Gen<TxnReferenceValue> valueGen(AbstractType<?> type)
+ {
+ return Generators.toGen(AbstractTypeGenerators.getTypeSupport(type)
+ .bytesGen())
+ .map(TxnReferenceValue.Constant::new);
+ }
+
+ private static Gen<TxnReferenceOperation> gen()
Review Comment:
ill review how generic this is and can push there.
```
table = table(type);
receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
```
that bit is hyper specific to this test, and not sure if we will ever actually have another user.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604069405
##########
test/harry/main/org/apache/cassandra/harry/model/PartitionState.java:
##########
@@ -410,6 +410,12 @@ public String toString(ValueGenerators valueGenerators)
", lts(" + StringUtils.toString(lts) + ")";
}
}
+
+ @Override
+ public String toString()
+ {
+ return toString(partitionState.valueGenerators);
Review Comment:
not related to this patch, but been in the other branch for ages...
when harry models detect a fault and show the rows, it was using the wrong toString which means that the descriptors won't match making it hard to see what happened...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #4518: CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly
URL: https://github.com/apache/cassandra/pull/4518
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2611534874
##########
src/java/org/apache/cassandra/service/accord/txn/TxnDataValue.java:
##########
@@ -41,22 +43,27 @@ interface TxnDataValueSerializer<T extends TxnDataValue> extends IVersionedSeria
enum Kind
{
key(0),
- range(1);
+ range(1),
+ message(2);
Review Comment:
https://github.com/apache/cassandra-accord/pull/262
1 out of the 4 callers of `execute` was after `result`, so flipped it like we talked about in slack.
With that I was able to drop `TxnData` knowing about failures and now we have a invalid rejection result
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612144458
##########
src/java/org/apache/cassandra/service/accord/txn/TxnUpdate.java:
##########
@@ -584,6 +588,25 @@ public static TxnUpdate empty()
return new TxnUpdate(TableMetadatas.none(), Keys.EMPTY, Collections.emptyList(), null, PreserveTimestamp.no);
}
+ public static TxnValidationRejection validationException(@Nullable Update update)
Review Comment:
```suggestion
@Nullable
public static TxnValidationRejection validationRejection(@Nullable Update update)
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604070678
##########
test/unit/org/apache/cassandra/cql3/ast/CollectionAccess.java:
##########
@@ -35,6 +35,18 @@ public CollectionAccess(ReferenceExpression column, Value element, AbstractType<
this.type = type;
}
+ @Override
+ public ReferenceExpression visit(Visitor v)
Review Comment:
when we rewrite the history to remove `Bind` this logic was missing so we get `?` which makes it hard to copy/paste to repo
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2615436998
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnReferenceOperationTest.java:
##########
@@ -0,0 +1,289 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Set;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import org.junit.Test;
+
+import accord.utils.Gen;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.FieldIdentifier;
+import org.apache.cassandra.cql3.Operation;
+import org.apache.cassandra.cql3.terms.Constants;
+import org.apache.cassandra.cql3.terms.UserTypes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.accord.serializers.TableMetadatas;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.Generators;
+import org.reflections.Reflections;
+import org.reflections.scanners.Scanners;
+import org.reflections.util.ConfigurationBuilder;
+
+import static accord.utils.Property.qt;
+
+public class TxnReferenceOperationTest
+{
+ private static final String KS = "ks";
+ private static final TxnData EMPTY = new TxnData();
+
+ @Test
+ public void coverage()
+ {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .forPackage("org.apache.cassandra")
+ .setScanners(Scanners.SubTypes)
+ .setExpandSuperTypes(true)
+ .setParallel(true));
+ var subTypes = reflections.getSubTypesOf(Operation.class);
+ var knownTypes = TxnReferenceOperation.initOperationKindMap().keySet();
+ // these types do not have a way to define a reference, as they are column level:
+ Set<Class<? extends Operation>> safeToExclude = ImmutableSet.of(Constants.Deleter.class, // DELETE foo
+ UserTypes.DeleterByField.class // DELETE foo.bar
+ );
+
+ StringBuilder sb = null;
+ for (var klass : Sets.difference(subTypes, Sets.union(knownTypes, safeToExclude)))
+ {
+ if (sb == null)
+ sb = new StringBuilder();
+ sb.append(klass.getCanonicalName()).append('\n');
+ }
+ if (sb != null)
+ throw new AssertionError(sb.toString());
+ }
+
+ @Test
+ public void serde()
+ {
+ @SuppressWarnings({ "resource", "IOResourceOpenedButNotSafelyClosed" }) DataOutputBuffer output = new DataOutputBuffer();
+ qt().withExamples(10_000).forAll(gen()).check(txnOp -> {
+ Serializers.testSerde(output, TxnReferenceOperation.serializer, txnOp, TableMetadatas.of(txnOp.table));
+
+ txnOp.toOperation(EMPTY);
+ });
+ }
+
+ private enum Group
+ {
+ Setter, SetterByIndex, SetterByKey, SetterByField,
+ Adder, Subtracter,
+ Appender, Putter, Discarder, Prepender,
+ }
+
+ private static Gen<TxnReferenceValue> valueGen(AbstractType<?> type)
+ {
+ return Generators.toGen(AbstractTypeGenerators.getTypeSupport(type)
+ .bytesGen())
+ .map(TxnReferenceValue.Constant::new);
+ }
+
+ private static Gen<TxnReferenceOperation> gen()
+ {
+ return rs -> {
+ TxnReferenceOperation.Kind kind;
+ ColumnMetadata receiver;
+ TableMetadata table;
+ @Nullable ByteBuffer keyOrIndex = null;
+ @Nullable ByteBuffer field = null;
+ TxnReferenceValue value;
+ Group group = rs.pick(Group.values());
+ switch (group)
+ {
+ case Prepender:
+ {
+ kind = TxnReferenceOperation.Kind.ListPrepender;
+ ListType<?> type = ListType.getInstance(Int32Type.instance, true);
+ value = valueGen(type).next(rs);
+
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ }
+ break;
+ case Discarder:
+ {
+ CollectionType<?> type = (CollectionType<?>) Generators.toGen(AbstractTypeGenerators.builder()
+ .withMaxDepth(1)
+ .withTypeKinds(AbstractTypeGenerators.TypeKind.LIST,
+ AbstractTypeGenerators.TypeKind.SET,
+ AbstractTypeGenerators.TypeKind.MAP)
+ .build())
+ .next(rs);
+ kind = type instanceof ListType
+ ? TxnReferenceOperation.Kind.ListDiscarder
+ : TxnReferenceOperation.Kind.SetDiscarder;
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ if (kind == TxnReferenceOperation.Kind.ListDiscarder && rs.nextBoolean())
+ {
+ kind = TxnReferenceOperation.Kind.ListDiscarderByIndex;
+ value = new TxnReferenceValue.Constant(Int32Type.instance.decompose(42));
+ }
+ else if (type instanceof MapType && rs.nextBoolean())
+ {
+ kind = TxnReferenceOperation.Kind.MapDiscarderByKey;
+ var keyType = ((MapType<?, ?>) type).getKeysType();
+ value = valueGen(keyType).next(rs);
+ }
+ else if (type instanceof SetType && rs.nextBoolean())
+ {
+ kind = TxnReferenceOperation.Kind.SetElementDiscarder;
+ var elementType = ((SetType<?>) type).getElementsType();
+ value = valueGen(elementType).next(rs);
+ }
+ else
+ {
+ CollectionType<?> discardType = type instanceof MapType
+ ? SetType.getInstance(((MapType<?, ?>) type).getKeysType(), true)
+ : type;
+ value = valueGen(discardType).next(rs);
+ }
+ }
+ break;
+ case Adder:
+ case Subtracter:
+ {
+ if (group == Group.Adder && rs.nextBoolean())
+ {
+ // this is similar to ListAppend
+ var type = SetType.getInstance(Int32Type.instance, true);
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(type).next(rs);
+ kind = TxnReferenceOperation.Kind.SetAdder;
+ }
+ else
+ {
+ var type = Int32Type.instance;
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(type).next(rs);
+ kind = group == Group.Adder ? TxnReferenceOperation.Kind.ConstantAdder : TxnReferenceOperation.Kind.ConstantSubtracter;
+ }
+ }
+ break;
+ case Setter:
+ {
+ var type = Generators.toGen(AbstractTypeGenerators.builder()
+ .withoutUnsafeEquality()
+ .withMaxDepth(1)
+ .build())
+ .next(rs);
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(type).next(rs);
+ if (type instanceof ListType)
+ kind = TxnReferenceOperation.Kind.ListSetter;
+ else if (type instanceof SetType)
+ kind = TxnReferenceOperation.Kind.SetSetter;
+ else if (type instanceof MapType)
+ kind = TxnReferenceOperation.Kind.MapSetter;
+ else if (type instanceof UserType)
+ kind = TxnReferenceOperation.Kind.UserTypeSetter;
+ else
+ kind = TxnReferenceOperation.Kind.ConstantSetter;
+ }
+ break;
+ case SetterByIndex:
+ {
+ ListType<String> type = ListType.getInstance(UTF8Type.instance, true);
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(type.getElementsType()).next(rs);
+ kind = TxnReferenceOperation.Kind.ListSetterByIndex;// x[?] = ?
+ keyOrIndex = Int32Type.instance.decompose(42);
+ }
+ break;
+ case Appender:
+ {
+ ListType<String> type = ListType.getInstance(UTF8Type.instance, true);
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(type).next(rs);
+ kind = TxnReferenceOperation.Kind.ListAppender;
+ }
+ break;
+ case SetterByKey:
+ {
+ MapType<Integer, String> type = MapType.getInstance(Int32Type.instance, UTF8Type.instance, true);
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(type.getValuesType()).next(rs);
+ kind = TxnReferenceOperation.Kind.MapSetterByKey;
+ keyOrIndex = Int32Type.instance.decompose(42);
+ }
+ break;
+ case Putter: // x += {foo: bar, baz: biz} -- basically Appender but for Map!
+ {
+ MapType<Integer, String> type = MapType.getInstance(Int32Type.instance, UTF8Type.instance, true);
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(type).next(rs);
+ kind = TxnReferenceOperation.Kind.MapPutter;
+ }
+ break;
+ case SetterByField:
+ {
+ UserType type = new UserType(KS, ByteBufferUtil.bytes("udt"),
+ List.of(FieldIdentifier.forUnquoted("f1")),
+ List.of(UTF8Type.instance),
+ true);
+ kind = TxnReferenceOperation.Kind.UserTypeSetterByField;
+ table = table(type);
+ receiver = table.getColumn(ColumnIdentifier.getInterned("col", true));
+ value = valueGen(UTF8Type.instance).next(rs);
+ field = FieldIdentifier.forUnquoted("f1").bytes;
+ }
+ break;
+
+ default:
+ throw new UnsupportedOperationException();
+ }
+ return new TxnReferenceOperation(kind, receiver, table, keyOrIndex, field, value);
+ };
+ }
+
+ private static TableMetadata table(AbstractType<?> type)
+ {
+ return TableMetadata.builder(KS, "tbl")
+ .partitioner(Murmur3Partitioner.instance)
+ .addPartitionKeyColumn("pk", Int32Type.instance)
+ .addRegularColumn("col", type)
+ .build();
+ }
+}
Review Comment:
Please add a new line at the end of the file.
##########
src/java/org/apache/cassandra/service/StorageProxy.java:
##########
@@ -1316,13 +1318,17 @@ public static void dispatchMutationsWithRetryOnDifferentSystem(List<? extends IM
{
if (accordResult != null)
{
- TxnResult.Kind kind = accordResult.awaitAndGet().kind();
+ TxnResult result = accordResult.awaitAndGet();
+ TxnResult.Kind kind = result.kind();
if (kind == retry_new_protocol && failure == null)
{
Tracing.trace("Accord returned retry new protocol");
logger.debug("Retrying mutations on different system because some mutations were misrouted according to Accord");
continue;
}
+ if (result.kind() == validation_rejection)
+ throw ((TxnValidationRejection) result).validationException;
+
Review Comment:
Similar if condition checks are present in several parts of the code. Can this be moved to a helper in `TransactionResult` to avoid code duplication?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2615534436
##########
src/java/org/apache/cassandra/service/StorageProxy.java:
##########
@@ -1316,13 +1318,17 @@ public static void dispatchMutationsWithRetryOnDifferentSystem(List<? extends IM
{
if (accordResult != null)
{
- TxnResult.Kind kind = accordResult.awaitAndGet().kind();
+ TxnResult result = accordResult.awaitAndGet();
+ TxnResult.Kind kind = result.kind();
if (kind == retry_new_protocol && failure == null)
{
Tracing.trace("Accord returned retry new protocol");
logger.debug("Retrying mutations on different system because some mutations were misrouted according to Accord");
continue;
}
+ if (result.kind() == validation_rejection)
+ throw ((TxnValidationRejection) result).validationException;
+
Review Comment:
switched to `TxnValidationRejection.maybeThrow(txnResult);`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604049073
##########
src/java/org/apache/cassandra/cql3/statements/TransactionStatement.java:
##########
@@ -250,7 +250,7 @@ TxnNamedRead createNamedRead(NamedSelect namedSelect, QueryOptions options, Tabl
SinglePartitionReadQuery.Group<SinglePartitionReadCommand> selectQuery = (SinglePartitionReadQuery.Group<SinglePartitionReadCommand>) select.getQuery(options, 0);
if (selectQuery.queries.size() != 1)
- throw new IllegalArgumentException("Within a transaction, SELECT statements must select a single partition; found " + selectQuery.queries.size() + " partitions");
+ throw invalidRequest("Within a transaction, SELECT statements must select a single partition; found " + selectQuery.queries.size() + " partitions");
Review Comment:
`IllegalArgumentException` is "server error" to users, so need `invalidRequest` to let the user know their query is at fault
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604067809
##########
test/harry/main/org/apache/cassandra/harry/model/BytesPartitionState.java:
##########
@@ -348,7 +348,7 @@ public String toString()
sb.append(partition);
sb.append(", clustering=");
if (clustering == null) sb.append("null");
- else appendValues(sb, factory.clusteringColumns, clustering);
+ else appendValues(sb, clustering == Clustering.STATIC_CLUSTERING ? List.of() : factory.clusteringColumns, clustering);
Review Comment:
static is special cased but we rely on the column name being empty...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2606138351
##########
src/java/org/apache/cassandra/batchlog/BatchlogManager.java:
##########
@@ -451,9 +453,12 @@ public void finish(Set<UUID> hintedNodes)
{
if (accordResult != null)
{
- TxnResult.Kind kind = accordResult.awaitAndGet().kind();
+ TxnResult result = accordResult.awaitAndGet();
+ TxnResult.Kind kind = result.kind();
if (kind == retry_new_protocol)
throw new RetryOnDifferentSystemException();
+ if (result.kind() == txn_data)
+ ((TxnData) result).checkAndThrowValidationException();
Review Comment:
I haven't looked at this carefully, but are we sure batch replay wants to throw an exception here? I'm not sure what the correct semantics are, and I won't worry about them much, just want to make sure this is considered.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604060879
##########
src/java/org/apache/cassandra/service/accord/txn/TxnReferenceOperation.java:
##########
@@ -202,27 +227,35 @@ public boolean equals(Object o)
TxnReferenceOperation that = (TxnReferenceOperation) o;
return Objects.equals(receiver, that.receiver)
&& kind == that.kind
- && Objects.equals(key, that.key)
+ && Objects.equals(keyOrIndex, that.keyOrIndex)
&& Objects.equals(field, that.field)
&& Objects.equals(value, that.value);
}
- public void collect(TableMetadatas.Collector collector)
- {
- collector.add(table);
- value.collect(collector);
- }
-
@Override
public int hashCode()
{
- return Objects.hash(receiver, kind, key, field, value);
+ return Objects.hash(receiver, kind, keyOrIndex, field, value);
}
+
@Override
public String toString()
{
- return receiver + " = " + value;
Review Comment:
this is only true for `Setter` and nothing else, so went with basic toString
##########
src/java/org/apache/cassandra/service/accord/txn/TxnReferenceOperation.java:
##########
@@ -274,9 +308,9 @@ public void serialize(TxnReferenceOperation operation, TableMetadatas tables, Da
columnMetadataSerializer.serialize(operation.receiver, operation.table, out);
TxnReferenceValue.serializer.serialize(operation.value, tables, out);
- out.writeBoolean(operation.key != null);
- if (operation.key != null)
- ByteBufferUtil.writeWithVIntLength(operation.key, out);
+ out.writeBoolean(operation.keyOrIndex != null);
Review Comment:
the bools were missing, so the serializer test detected we were off by 2 bytes
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604064459
##########
src/java/org/apache/cassandra/service/accord/txn/TxnUpdate.java:
##########
@@ -677,7 +678,18 @@ public TxnWrite apply(Timestamp executeAt, Data data)
ClusterMetadata cm = ClusterMetadata.current();
checkState(cm.epoch.getEpoch() >= executeAt.epoch(), "TCM epoch %d is < executeAt epoch %d", cm.epoch.getEpoch(), executeAt.epoch());
- Pair<List<TxnWrite.Update>, SimpleBitSet> pair = processCondition(executeAt, data);
+ Pair<List<TxnWrite.Update>, SimpleBitSet> pair;
+ try
+ {
+ pair = processCondition(executeAt, data);
+ }
+ catch (RequestValidationException e)
+ {
+ // the update isn't allowed
+ ((TxnData) data).putValidationException(e.code(), e.getMessage());
Review Comment:
i didn't test constraints, i *hope* they are `RequestValidationException` but didn't check
##########
test/distributed/org/apache/cassandra/distributed/shared/ClusterUtils.java:
##########
@@ -717,6 +719,8 @@ public static Epoch maxEpoch(ICluster<IInvokableInstance> cluster)
public static Epoch maxEpoch(ICluster<IInvokableInstance> cluster, int... nodes)
{
+ if (nodes == null || nodes.length == 0)
Review Comment:
this bug has been in the other branch for ages and adds a race condition to tests that depend on this... so not "part" of this patch, but makes sure we stop hitting these race conditions
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612054365
##########
test/distributed/org/apache/cassandra/distributed/test/accord/AccordCQLTestBase.java:
##########
@@ -3415,4 +3415,20 @@ public void multiPartitionUpdate() throws Exception
}
});
}
+
+ @Test
+ public void userSeesInvalidRejection() throws Exception
+ {
+ var expectedType = AssertionUtils.isInstanceof(InvalidRequestException.class);
+ test("CREATE TABLE " + qualifiedAccordTableName + "(k int PRIMARY KEY, l list<int>) WITH " + transactionalMode.asCqlParam(), cluster -> {
+ String cql = "UPDATE " + qualifiedAccordTableName + " SET l[0] = 42 WHERE k=42";
+ Assertions.assertThatThrownBy(() -> cluster.coordinator(1).execute(cql, QUORUM))
+ .is(expectedType)
+ .hasMessage("Attempted to set an element on a list which is null");
+
+ Assertions.assertThatThrownBy(() -> cluster.coordinator(1).execute(wrapInTxn(cql), QUORUM))
+ .is(expectedType)
+ .hasMessage("Attempted to set an element on a list which is null");
Review Comment:
nit: It would be nice to not duplicate the error message text I guess, but we don't seem to be in the habit of doing that in `Lists`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2608317296
##########
src/java/org/apache/cassandra/batchlog/BatchlogManager.java:
##########
@@ -451,9 +453,12 @@ public void finish(Set<UUID> hintedNodes)
{
if (accordResult != null)
{
- TxnResult.Kind kind = accordResult.awaitAndGet().kind();
+ TxnResult result = accordResult.awaitAndGet();
+ TxnResult.Kind kind = result.kind();
if (kind == retry_new_protocol)
throw new RetryOnDifferentSystemException();
+ if (result.kind() == txn_data)
+ ((TxnData) result).checkAndThrowValidationException();
Review Comment:
moved to `Result` based but accord isn't consistent and `execute` *happens after* `result` in the common case; if we can flip this order than this result logic isn't so hacky
##########
src/java/org/apache/cassandra/service/accord/txn/TxnDataValue.java:
##########
@@ -41,22 +43,27 @@ interface TxnDataValueSerializer<T extends TxnDataValue> extends IVersionedSeria
enum Kind
{
key(0),
- range(1);
+ range(1),
+ message(2);
Review Comment:
moved to `Result` based but accord isn't consistent and `execute` *happens after* `result` in the common case; if we can flip this order than this result logic isn't so hacky
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2611998986
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnReferenceOperationTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Set;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import accord.utils.Gen;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.FieldIdentifier;
+import org.apache.cassandra.cql3.Operation;
+import org.apache.cassandra.cql3.terms.Constants;
+import org.apache.cassandra.cql3.terms.UserTypes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.accord.serializers.TableMetadatas;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.Generators;
+import org.reflections.Reflections;
+import org.reflections.scanners.Scanners;
+import org.reflections.util.ConfigurationBuilder;
+
+import static accord.utils.Property.qt;
+
+public class TxnReferenceOperationTest
+{
+ private static final String KS = "ks";
+ private static final TxnData EMPTY = new TxnData();
+
+ @Test
+ public void coverage()
+ {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .forPackage("org.apache.cassandra")
+ .setScanners(Scanners.SubTypes)
+ .setExpandSuperTypes(true)
+ .setParallel(true));
+ var subTypes = reflections.getSubTypesOf(Operation.class);
+ var knownTypes = TxnReferenceOperation.initOperationKindMap().keySet();
+ // these types do not have a way to define a reference, as they are column level:
+ Set<Class<? extends Operation>> safeToExclude = ImmutableSet.of(Constants.Deleter.class, // DELETE foo
+ UserTypes.DeleterByField.class // DELETE foo.bar
+ );
+
+ StringBuilder sb = null;
+ for (var klass : com.google.common.collect.Sets.difference(subTypes, com.google.common.collect.Sets.union(knownTypes, safeToExclude)))
Review Comment:
nit: Possible to just throw up a `import com.google.common.collect.Sets;` and avoid the fully qualified name?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612147919
##########
src/java/org/apache/cassandra/service/accord/txn/TxnUpdate.java:
##########
@@ -677,7 +700,19 @@ public TxnWrite apply(Timestamp executeAt, Data data)
ClusterMetadata cm = ClusterMetadata.current();
checkState(cm.epoch.getEpoch() >= executeAt.epoch(), "TCM epoch %d is < executeAt epoch %d", cm.epoch.getEpoch(), executeAt.epoch());
- Pair<List<TxnWrite.Update>, SimpleBitSet> pair = processCondition(executeAt, data);
+ Pair<List<TxnWrite.Update>, SimpleBitSet> pair;
Review Comment:
```suggestion
Pair<List<TxnWrite.Update>, SimpleBitSet> pair = null;
```
...and then you don't need the assignment in the `catch` block?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2612234180
##########
test/unit/org/apache/cassandra/service/accord/txn/TxnReferenceOperationTest.java:
##########
@@ -0,0 +1,288 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.service.accord.txn;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Set;
+
+import javax.annotation.Nullable;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import accord.utils.Gen;
+import org.apache.cassandra.cql3.ColumnIdentifier;
+import org.apache.cassandra.cql3.FieldIdentifier;
+import org.apache.cassandra.cql3.Operation;
+import org.apache.cassandra.cql3.terms.Constants;
+import org.apache.cassandra.cql3.terms.UserTypes;
+import org.apache.cassandra.db.marshal.AbstractType;
+import org.apache.cassandra.db.marshal.CollectionType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.ListType;
+import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.SetType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UserType;
+import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.io.Serializers;
+import org.apache.cassandra.io.util.DataOutputBuffer;
+import org.apache.cassandra.schema.ColumnMetadata;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.accord.serializers.TableMetadatas;
+import org.apache.cassandra.utils.AbstractTypeGenerators;
+import org.apache.cassandra.utils.ByteBufferUtil;
+import org.apache.cassandra.utils.Generators;
+import org.reflections.Reflections;
+import org.reflections.scanners.Scanners;
+import org.reflections.util.ConfigurationBuilder;
+
+import static accord.utils.Property.qt;
+
+public class TxnReferenceOperationTest
+{
+ private static final String KS = "ks";
+ private static final TxnData EMPTY = new TxnData();
+
+ @Test
+ public void coverage()
+ {
+ Reflections reflections = new Reflections(new ConfigurationBuilder()
+ .forPackage("org.apache.cassandra")
+ .setScanners(Scanners.SubTypes)
+ .setExpandSuperTypes(true)
+ .setParallel(true));
+ var subTypes = reflections.getSubTypesOf(Operation.class);
+ var knownTypes = TxnReferenceOperation.initOperationKindMap().keySet();
+ // these types do not have a way to define a reference, as they are column level:
+ Set<Class<? extends Operation>> safeToExclude = ImmutableSet.of(Constants.Deleter.class, // DELETE foo
+ UserTypes.DeleterByField.class // DELETE foo.bar
+ );
+
+ StringBuilder sb = null;
+ for (var klass : com.google.common.collect.Sets.difference(subTypes, com.google.common.collect.Sets.union(knownTypes, safeToExclude)))
Review Comment:
doh, that was removed... so no more conflict it seems!
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2604056060
##########
src/java/org/apache/cassandra/service/accord/txn/TxnReferenceOperation.java:
##########
@@ -62,21 +68,22 @@ public class TxnReferenceOperation
private static Map<Class<? extends Operation>, Kind> initOperationKindMap()
{
Map<Class<? extends Operation>, Kind> temp = new HashMap<>();
- temp.put(Sets.Adder.class, Kind.SetAdder);
Review Comment:
i reorded so its easier to see something missing. `Lists.DiscarderByIndex` was missing!
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21061: Accord: write rejections would be returned to users as server errors rather than INVALID and TxnReferenceOperation didnt handle all collections properly [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #4518:
URL: https://github.com/apache/cassandra/pull/4518#discussion_r2619352071
##########
src/java/org/apache/cassandra/service/accord/txn/TxnUpdate.java:
##########
@@ -677,7 +678,18 @@ public TxnWrite apply(Timestamp executeAt, Data data)
ClusterMetadata cm = ClusterMetadata.current();
checkState(cm.epoch.getEpoch() >= executeAt.epoch(), "TCM epoch %d is < executeAt epoch %d", cm.epoch.getEpoch(), executeAt.epoch());
- Pair<List<TxnWrite.Update>, SimpleBitSet> pair = processCondition(executeAt, data);
+ Pair<List<TxnWrite.Update>, SimpleBitSet> pair;
+ try
+ {
+ pair = processCondition(executeAt, data);
+ }
+ catch (RequestValidationException e)
+ {
+ // the update isn't allowed
+ ((TxnData) data).putValidationException(e.code(), e.getMessage());
Review Comment:
they are
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org