You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "smiklosovic (via GitHub)" <gi...@apache.org> on 2024/09/10 12:29:48 UTC
[PR] CASSANDRA-19906 make memtable configuration order-independent [cassandra]
smiklosovic opened a new pull request, #3536:
URL: https://github.com/apache/cassandra/pull/3536
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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-19906 make memtable configuration order-independent [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #3536:
URL: https://github.com/apache/cassandra/pull/3536#issuecomment-2341748443
I would recommend adding the following patch as it improves the test coverage. In `CQLTester.Fuzzed` we will create random yaml configs and includes this layer. The patch makes sure to test with randomized ordering (and finds the bug reported), and includes a fuzz test showing your changes are stable and work as expected
```
diff --git a/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java b/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java
index 7db5204a9f..d12dcb556f 100644
--- a/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java
+++ b/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java
@@ -20,7 +20,11 @@ package org.apache.cassandra.schema;
import java.util.Map;
+import accord.utils.Gen;
import com.google.common.collect.ImmutableMap;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.utils.ConfigGenBuilder;
+import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;
@@ -29,172 +33,187 @@ import org.apache.cassandra.config.ParameterizedClass;
import org.apache.cassandra.db.memtable.SkipListMemtableFactory;
import org.apache.cassandra.exceptions.ConfigurationException;
+import static accord.utils.Property.qt;
+import static org.apache.cassandra.config.YamlConfigurationLoader.fromMap;
import static org.junit.Assert.assertEquals;
-public class MemtableParamsTest
-{
+public class MemtableParamsTest {
static final ParameterizedClass DEFAULT = SkipListMemtableFactory.CONFIGURATION;
@Test
- public void testDefault()
- {
+ public void testDefault() {
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions(ImmutableMap.of());
assertEquals(ImmutableMap.of("default", DEFAULT), map);
}
@Test
- public void testDefaultRemapped()
- {
+ public void testDefaultRemapped() {
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions
- (
- ImmutableMap.of("remap", new InheritingClass("default", null, null))
- );
+ (
+ ImmutableMap.of("remap", new InheritingClass("default", null, null))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "remap", DEFAULT),
- map);
+ "remap", DEFAULT),
+ map);
}
@Test
- public void testOne()
- {
+ public void testOne() {
final InheritingClass one = new InheritingClass(null, "SkipList", null);
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions(ImmutableMap.of("one", one));
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one),
- map);
+ "one", one),
+ map);
}
@Test
- public void testExtends()
- {
+ public void testExtends() {
final InheritingClass one = new InheritingClass(null, "SkipList", null);
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "two", new InheritingClass("one",
- null,
- ImmutableMap.of("extra", "value")))
- );
+ (
+ ImmutableMap.of("one", one,
+ "two", new InheritingClass("one",
+ null,
+ ImmutableMap.of("extra", "value")))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one,
- "two", new ParameterizedClass("SkipList",
- ImmutableMap.of("extra", "value"))),
- map);
+ "one", one,
+ "two", new ParameterizedClass("SkipList",
+ ImmutableMap.of("extra", "value"))),
+ map);
}
@Test
- public void testExtendsReplace()
- {
+ public void testExtendsReplace() {
final InheritingClass one = new InheritingClass(null,
- "SkipList",
- ImmutableMap.of("extra", "valueOne"));
+ "SkipList",
+ ImmutableMap.of("extra", "valueOne"));
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "two", new InheritingClass("one",
- null,
- ImmutableMap.of("extra", "value")))
- );
+ (
+ ImmutableMap.of("one", one,
+ "two", new InheritingClass("one",
+ null,
+ ImmutableMap.of("extra", "value")))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one,
- "two", new ParameterizedClass("SkipList",
- ImmutableMap.of("extra", "value"))),
- map);
+ "one", one,
+ "two", new ParameterizedClass("SkipList",
+ ImmutableMap.of("extra", "value"))),
+ map);
}
@Test
- public void testDoubleExtends()
- {
+ public void testDoubleExtends() {
final InheritingClass one = new InheritingClass(null, "SkipList", null);
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "two", new InheritingClass("one",
- null,
- ImmutableMap.of("param", "valueTwo",
- "extra", "value")),
- "three", new InheritingClass("two",
- "OtherClass",
- ImmutableMap.of("param", "valueThree",
- "extraThree", "three")))
- );
+ (
+ ImmutableMap.of("one", one,
+ "two", new InheritingClass("one",
+ null,
+ ImmutableMap.of("param", "valueTwo",
+ "extra", "value")),
+ "three", new InheritingClass("two",
+ "OtherClass",
+ ImmutableMap.of("param", "valueThree",
+ "extraThree", "three")))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one,
- "two", new ParameterizedClass("SkipList",
- ImmutableMap.of("param", "valueTwo",
- "extra", "value")),
- "three", new ParameterizedClass("OtherClass",
- ImmutableMap.of("param", "valueThree",
- "extra", "value",
- "extraThree", "three"))),
- map);
+ "one", one,
+ "two", new ParameterizedClass("SkipList",
+ ImmutableMap.of("param", "valueTwo",
+ "extra", "value")),
+ "three", new ParameterizedClass("OtherClass",
+ ImmutableMap.of("param", "valueThree",
+ "extra", "value",
+ "extraThree", "three"))),
+ map);
}
@Test
- public void testInvalidExtends()
- {
+ public void testInvalidExtends() {
final InheritingClass one = new InheritingClass(null, "SkipList", null);
- try
- {
+ try {
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions
- (
- ImmutableMap.of("two", new InheritingClass("one",
- null,
- ImmutableMap.of("extra", "value")),
- "one", one)
- );
+ (
+ ImmutableMap.of("two", new InheritingClass("one",
+ null,
+ ImmutableMap.of("extra", "value")),
+ "one", one)
+ );
Assert.fail("Expected exception.");
- }
- catch (ConfigurationException e)
- {
+ } catch (ConfigurationException e) {
// expected
}
}
@Test
- public void testInvalidSelfExtends()
- {
- try
- {
+ public void testInvalidSelfExtends() {
+ try {
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", new InheritingClass("one",
- null,
- ImmutableMap.of("extra", "value")))
- );
+ (
+ ImmutableMap.of("one", new InheritingClass("one",
+ null,
+ ImmutableMap.of("extra", "value")))
+ );
Assert.fail("Expected exception.");
- }
- catch (ConfigurationException e)
- {
+ } catch (ConfigurationException e) {
// expected
}
}
@Test
- public void testReplaceDefault()
- {
+ public void testReplaceDefault() {
final InheritingClass one = new InheritingClass(null,
- "SkipList",
- ImmutableMap.of("extra", "valueOne"));
+ "SkipList",
+ ImmutableMap.of("extra", "valueOne"));
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions(ImmutableMap.of("default", one));
assertEquals(ImmutableMap.of("default", one), map);
}
@Test
- public void testDefaultExtends()
- {
+ public void testDefaultExtends() {
final InheritingClass one = new InheritingClass(null,
- "SkipList",
- ImmutableMap.of("extra", "valueOne"));
+ "SkipList",
+ ImmutableMap.of("extra", "valueOne"));
Map<String, ParameterizedClass> map = MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "default", new InheritingClass("one", null, ImmutableMap.of()))
- );
+ (
+ ImmutableMap.of("one", one,
+ "default", new InheritingClass("one", null, ImmutableMap.of()))
+ );
assertEquals(ImmutableMap.of("one", one,
- "default", one),
- map);
+ "default", one),
+ map);
}
// Note: The factories constructed from these parameters are tested in the CreateTest and AlterTest.
+
+ @Test
+ public void testExpandDefinitions() {
+ Gen<Map<String, InheritingClass>> gen = new ConfigGenBuilder()
+ .buildMemtable()
+ .map(m -> fromMap(m, Config.class))
+ .map(c -> c.memtable.configurations);
+ qt().forAll(gen).check(configs -> {
+ Map<String, ParameterizedClass> result = MemtableParams.expandDefinitions(configs);
+ Assertions.assertThat(result.keySet()).isEqualTo(configs.keySet());
+ for (Map.Entry<String, InheritingClass> e : configs.entrySet()) {
+ if (e.getValue().inherits == null) continue;
+ ParameterizedClass parent = result.get(e.getValue().inherits);
+ ParameterizedClass src = result.get(e.getKey());
+ Assertions.assertThat(src.class_name).isEqualTo(parent.class_name);
+ Assertions.assertThat(src.parameters).isEqualTo(parent.parameters);
+ }
+ });
+ }
+
+ @Test
+ public void testInheritsUnresolved() {
+ MemtableParams.expandDefinitions(Map.of("a", inherits("b"),
+ "b", inherits("a")));
+ }
+
+ private static InheritingClass inherits(String name)
+ {
+ return new InheritingClass(name, null, null);
+ }
}
diff --git a/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java b/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java
index e0b89e767c..f339e42522 100644
--- a/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java
+++ b/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java
@@ -18,9 +18,7 @@
package org.apache.cassandra.utils;
-import java.util.LinkedHashMap;
-import java.util.Map;
-import java.util.Objects;
+import java.util.*;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
@@ -148,24 +146,65 @@ public class ConfigGenBuilder
config.put("commitlog_disk_access_mode", commitLogDiskAccessModeGen.next(rs));
}
- private void updateConfigMemtable(RandomSource rs, Map<String, Object> config)
+ public Gen<Map<String, Object>> buildMemtable()
+ {
+ return rs -> {
+ LinkedHashMap<String, Object> config = new LinkedHashMap<>();
+ updateConfigMemtable(rs, config);
+ return config;
+ };
+ }
+
+ private enum MemtableConfigShape { Single, Simple, Nested}
+ public void updateConfigMemtable(RandomSource rs, Map<String, Object> config)
{
config.put("memtable_allocation_type", memtableAllocationTypeGen.next(rs));
Memtable defaultMemtable = memtableGen.next(rs);
- Map<String, Map<String, Object>> memtables = new LinkedHashMap<>();
- if (rs.nextBoolean())
+ LinkedHashMap<String, Map<String, Object>> memtables = new LinkedHashMap<>();
+ switch (rs.pick(MemtableConfigShape.values()))
{
- // use inherits
- for (Memtable m : Memtable.values())
- memtables.put(m.name(), createConfig(m).next(rs));
- memtables.put("default", ImmutableMap.of("inherits", defaultMemtable.name()));
- }
- else
- {
- // define inline
- memtables.put("default", createConfig(defaultMemtable).next(rs));
+ case Single:
+ // define inline
+ memtables.put("default", createConfig(defaultMemtable).next(rs));
+ break;
+ case Simple:
+ // use inherits
+ for (Memtable m : Memtable.values())
+ memtables.put(m.name(), createConfig(m).next(rs));
+ memtables.put("default", ImmutableMap.of("inherits", defaultMemtable.name()));
+ break;
+ case Nested:
+ {
+ for (Memtable m : Memtable.values())
+ {
+ int levels = rs.nextInt(0, 4);
+ String prev = m.name() + "_base";
+ memtables.put(prev, createConfig(m).next(rs));
+ for (int i = 0; i < levels; i++)
+ {
+ String next = m.name() + '_' + i;
+ memtables.put(next, ImmutableMap.of("inherits", prev));
+ prev = next;
+ }
+ memtables.put(m.name(), ImmutableMap.of("inherits", prev));
+ }
+ memtables.put("default", ImmutableMap.of("inherits", defaultMemtable.name()));
+ }
+ break;
+ default:
+ throw new UnsupportedOperationException();
}
- config.put("memtable", ImmutableMap.of("configurations", memtables));
+ config.put("memtable", ImmutableMap.of("configurations", scrable(rs, memtables)));
+ }
+
+ private static <K, V> LinkedHashMap<K, V> scrable(RandomSource rs, LinkedHashMap<K, V> map)
+ {
+ List<K> keys = new ArrayList<>(map.keySet());
+ Collections.shuffle(keys, rs.asJdkRandom());
+ LinkedHashMap<K, V> copy = new LinkedHashMap<>();
+ for (K key : keys)
+ copy.put(key, map.get(key));
+ return copy;
}
private static Gen<Map<String, Object>> createConfig(Memtable type)
```
--
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-19906 make memtable configuration order-independent [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #3536: CASSANDRA-19906 make memtable configuration order-independent
URL: https://github.com/apache/cassandra/pull/3536
--
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-19906 make memtable configuration order-independent [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #3536:
URL: https://github.com/apache/cassandra/pull/3536#discussion_r1752333384
##########
src/java/org/apache/cassandra/schema/MemtableParams.java:
##########
@@ -147,8 +148,65 @@ static Map<String, ParameterizedClass> expandDefinitions(Map<String, InheritingC
if (!memtableConfigurations.containsKey(DEFAULT_CONFIGURATION_KEY))
configs.put(DEFAULT_CONFIGURATION_KEY, DEFAULT_CONFIGURATION);
- for (Map.Entry<String, InheritingClass> en : memtableConfigurations.entrySet())
- configs.put(en.getKey(), en.getValue().resolve(configs));
+ // resolve default always as the last one, see CASSANDRA-19906
+
+ Map<String, InheritingClass> inheritingClasses = new LinkedHashMap<>();
+
+ Map.Entry<String, InheritingClass> defaultConfiguration = null;
+
+ for (Map.Entry<String, InheritingClass> entry : memtableConfigurations.entrySet())
+ {
+ if (entry.getValue().inherits != null)
+ {
+ if (entry.getKey().equals(entry.getValue().inherits))
+ throw new ConfigurationException(String.format("Configuration entry %s can not inherit itself.", entry.getKey()));
+
+ if (memtableConfigurations.get(entry.getValue().inherits) == null && !entry.getValue().inherits.equals(DEFAULT_CONFIGURATION_KEY))
+ throw new ConfigurationException(String.format("Configuration entry %s inherits non-existing entry %s.",
+ entry.getKey(), entry.getValue().inherits));
+
+ inheritingClasses.put(entry.getKey(), entry.getValue());
+ }
+ else if (entry.getKey().equals(DEFAULT_CONFIGURATION_KEY))
+ defaultConfiguration = entry;
+ else
+ configs.put(entry.getKey(), entry.getValue().resolve(configs));
+ }
+
+ for (Map.Entry<String, InheritingClass> inheritingEntry : inheritingClasses.entrySet())
+ {
+ String inherits = inheritingEntry.getValue().inherits;
+ while (inherits != null)
+ {
+ InheritingClass nextInheritance = inheritingClasses.get(inherits);
+ if (nextInheritance == null)
+ inherits = null;
+ else
+ inherits = nextInheritance.inherits;
+
+ if (inherits != null && inherits.equals(inheritingEntry.getKey()))
+ throw new ConfigurationException(String.format("Detected loop when processing key %s", inheritingEntry.getKey()));
+ }
+ }
+
+ while (!inheritingClasses.isEmpty())
+ {
+ Set<String> forRemoval = new HashSet<>();
+ for (Map.Entry<String, InheritingClass> inheritingEntry : inheritingClasses.entrySet())
+ {
+ if (configs.get(inheritingEntry.getValue().inherits) != null)
+ {
+ configs.put(inheritingEntry.getKey(), inheritingEntry.getValue().resolve(configs));
+ forRemoval.add(inheritingEntry.getKey());
+ }
+ }
+
+ for (String toRemove : forRemoval)
Review Comment:
this shouldn't happen but if `forRemoval` is empty then we loop forever... can we add an assert just in case? I can't see a way to hit this, its more to be paranoid than anything
--
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-19906 make memtable configuration order-independent [cassandra]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #3536:
URL: https://github.com/apache/cassandra/pull/3536#issuecomment-2340646439
This patch fixes a very specific and common case by I don’t think is enough. The following is valid and not solved by this patch.
(On phone so real format is hard)
```
skiplist: class
skiplist_a:
inherit: skiplist
property:
foo: bar
default:
inherit: skiplist_a
```
a complete version would check for inheritances, see if the value is already added, else ensue until possible
--
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