You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/04/25 16:37:44 UTC
[GitHub] [cassandra] adelapena commented on a diff in pull request #1588: CASSANDRA-17456 trunk: Reject oversized mutations on client and internode connection
adelapena commented on code in PR #1588:
URL: https://github.com/apache/cassandra/pull/1588#discussion_r857817469
##########
test/unit/org/apache/cassandra/db/commitlog/CommitLogTest.java:
##########
@@ -476,69 +476,6 @@ public void testEqualRecordLimit() throws Exception
CommitLog.instance.add(rm);
}
- @Test(expected = MutationExceededMaxSizeException.class)
- public void testExceedRecordLimit() throws Exception
- {
- Keyspace ks = Keyspace.open(KEYSPACE1);
- ColumnFamilyStore cfs = ks.getColumnFamilyStore(STANDARD1);
- Mutation rm = new RowUpdateBuilder(cfs.metadata(), 0, "k")
- .clustering("bytes")
- .add("val", ByteBuffer.allocate(1 + getMaxRecordDataSize()))
- .build();
- long cnt = CommitLog.instance.metrics.oversizedMutations.getCount();
- try
- {
- CommitLog.instance.add(rm);
- }
- catch (MutationExceededMaxSizeException e)
- {
- assertEquals(cnt + 1, CommitLog.instance.metrics.oversizedMutations.getCount());
- throw e;
- }
- throw new AssertionError("mutation larger than limit was accepted");
- }
- @Test
- public void testExceedRecordLimitWithMultiplePartitions() throws Exception
- {
- CommitLog.instance.resetUnsafe(true);
- List<Mutation> mutations = new ArrayList<>();
- Keyspace ks = Keyspace.open(KEYSPACE1);
- char[] keyChars = new char[MutationExceededMaxSizeException.PARTITION_MESSAGE_LIMIT];
- Arrays.fill(keyChars, 'k');
- String key = new String(keyChars);
-
- // large mutation
- mutations.add(new RowUpdateBuilder(ks.getColumnFamilyStore(STANDARD1).metadata(), 0, key)
- .clustering("bytes")
- .add("val", ByteBuffer.allocate(1 + getMaxRecordDataSize()))
- .build());
-
- // smaller mutation
- mutations.add(new RowUpdateBuilder(ks.getColumnFamilyStore(STANDARD2).metadata(), 0, key)
- .clustering("bytes")
- .add("val", ByteBuffer.allocate(1 + getMaxRecordDataSize() - 1024))
- .build());
-
- Mutation mutation = Mutation.merge(mutations);
- try
- {
- CommitLog.instance.add(Mutation.merge(mutations));
- throw new AssertionError("mutation larger than limit was accepted");
- }
- catch (MutationExceededMaxSizeException exception)
- {
- String message = exception.getMessage();
-
- long mutationSize = mutation.serializedSize(MessagingService.current_version) + ENTRY_OVERHEAD_SIZE;
Review Comment:
Nit: this leaves the static import of `ENTRY_OVERHEAD_SIZE` usued.
##########
test/distributed/org/apache/cassandra/distributed/test/OversizedMutationTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.distributed.test;
+
+import org.apache.commons.lang3.StringUtils;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.Cluster;
+
+import static org.apache.cassandra.distributed.api.ConsistencyLevel.ALL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class OversizedMutationTest extends TestBaseImpl
+{
+ @Test
+ public void testSingleOversizedMutation() throws Throwable
+ {
+ try (Cluster cluster = init(builder().withNodes(1).withConfig(c -> c.set("max_mutation_size_in_kb", 48))
+ .start()))
+ {
+ cluster.schemaChange(withKeyspace("CREATE TABLE %s.t (key int PRIMARY KEY, val blob)"));
+ String payload = StringUtils.repeat('1', 1024 * 49);
+ String query = "INSERT INTO %s.t (key, val) VALUES (1, textAsBlob('" + payload + "'))";
+ try
+ {
+ cluster.coordinator(1).execute(withKeyspace(query), ALL);
+ fail("An error should have been thrown but was not.");
+ }
+ catch (Throwable e)
+ {
+ String msg = "Rejected an oversized mutation (50233/49152) for keyspace: distributed_test_keyspace. Top keys are: t.1";
+ assertEquals(msg, e.getMessage());
+ }
Review Comment:
The actual size of `50233` might be a bit fragile if we change serialization formats, maybe we could omit it and do something like:
```java
Assertions.assertThatThrownBy(() -> cluster.coordinator(1).execute(withKeyspace(query), ALL))
.hasMessageContaining("Rejected an oversized mutation (")
.hasMessageContaining("49152) for keyspace: distributed_test_keyspace. Top keys are: t.1");
```
wdyt? The same would apply to the other test for batches below.
##########
test/distributed/org/apache/cassandra/distributed/test/OversizedMutationTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.distributed.test;
+
+import org.apache.commons.lang3.StringUtils;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.Cluster;
+
+import static org.apache.cassandra.distributed.api.ConsistencyLevel.ALL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class OversizedMutationTest extends TestBaseImpl
+{
+ @Test
+ public void testSingleOversizedMutation() throws Throwable
+ {
+ try (Cluster cluster = init(builder().withNodes(1).withConfig(c -> c.set("max_mutation_size_in_kb", 48))
+ .start()))
+ {
+ cluster.schemaChange(withKeyspace("CREATE TABLE %s.t (key int PRIMARY KEY, val blob)"));
+ String payload = StringUtils.repeat('1', 1024 * 49);
+ String query = "INSERT INTO %s.t (key, val) VALUES (1, textAsBlob('" + payload + "'))";
+ try
+ {
+ cluster.coordinator(1).execute(withKeyspace(query), ALL);
+ fail("An error should have been thrown but was not.");
+ }
+ catch (Throwable e)
+ {
+ String msg = "Rejected an oversized mutation (50233/49152) for keyspace: distributed_test_keyspace. Top keys are: t.1";
+ assertEquals(msg, e.getMessage());
+ }
+ }
+ }
+
+ @Test
+ public void testOversizedBatch() throws Throwable
+ {
+ try (Cluster cluster = init(builder().withNodes(1).withConfig(c -> c.set("max_mutation_size_in_kb", 48))
+ .start()))
+ {
+ cluster.schemaChange(withKeyspace("CREATE KEYSPACE ks1 WITH replication = {'class': 'SimpleStrategy', 'replication_factor': 1};"));
+ cluster.schemaChange(withKeyspace("CREATE TABLE ks1.t (key int PRIMARY KEY, val blob)"));
+ String payload = StringUtils.repeat('1', 1024 * 48);
+ String query = "BEGIN BATCH\n" +
+ "INSERT INTO ks1.t (key, val) VALUES (1, textAsBlob('" + payload + "'))\n" +
+ "INSERT INTO ks1.t (key, val) VALUES (2, textAsBlob('222'))\n" +
+ "APPLY BATCH";
+ try
+ {
+ cluster.coordinator(1).execute(withKeyspace(query), ALL);
+ fail("An error should have been thrown but was not.");
+ }
+ catch (Throwable e)
+ {
+ String msg = "Rejected an oversized mutation (49209/49152) for keyspace: ks1. Top keys are: t.1";
+ assertEquals(msg, e.getMessage());
+ }
Review Comment:
Nit: this can be slightly simplified using `assertThatThrownBy`:
```suggestion
Assertions.assertThatThrownBy(() -> cluster.coordinator(1).execute(withKeyspace(query), ALL))
.hasMessage("Rejected an oversized mutation (49209/49152) for keyspace: ks1. Top keys are: t.1");
```
##########
test/distributed/org/apache/cassandra/distributed/test/OversizedMutationTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.distributed.test;
+
+import org.apache.commons.lang3.StringUtils;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.Cluster;
+
+import static org.apache.cassandra.distributed.api.ConsistencyLevel.ALL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+public class OversizedMutationTest extends TestBaseImpl
+{
+ @Test
+ public void testSingleOversizedMutation() throws Throwable
+ {
+ try (Cluster cluster = init(builder().withNodes(1).withConfig(c -> c.set("max_mutation_size_in_kb", 48))
+ .start()))
+ {
+ cluster.schemaChange(withKeyspace("CREATE TABLE %s.t (key int PRIMARY KEY, val blob)"));
+ String payload = StringUtils.repeat('1', 1024 * 49);
+ String query = "INSERT INTO %s.t (key, val) VALUES (1, textAsBlob('" + payload + "'))";
+ try
+ {
+ cluster.coordinator(1).execute(withKeyspace(query), ALL);
+ fail("An error should have been thrown but was not.");
+ }
+ catch (Throwable e)
+ {
+ String msg = "Rejected an oversized mutation (50233/49152) for keyspace: distributed_test_keyspace. Top keys are: t.1";
+ assertEquals(msg, e.getMessage());
+ }
Review Comment:
Nit: this can be slightly simplified using `assertThatThrownBy`:
```suggestion
Assertions.assertThatThrownBy(() -> cluster.coordinator(1).execute(withKeyspace(query), ALL))
.hasMessage("Rejected an oversized mutation (50233/49152) for keyspace: distributed_test_keyspace. Top keys are: t.1");
```
--
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