CASSSIDECAR-424: Add ConfigurationProvider interfaces by pauloricardomg · Pull Request #351 · apache/cassandra-sidecar

pauloricardomg · 2026-05-15T22:42:13Z

Summary

Introduces the pluggable overlay storage abstraction as defined in CEP-62 for managing Cassandra configuration via Sidecar.

CassandraConfigurationOverlay: Immutable model with version-agnostic cassandraYaml (JsonNode) and extraJvmOpts, with updated() centralizing merge logic for both components
ConfigurationOverlaySnapshot: Wrapper with lazily computed cached SHA-256 content hash and last modified timestamp
ConfigurationProvider: Pluggable storage interface with getConfiguration and storeConfiguration using hash-based optimistic concurrency
InMemoryConfigurationProvider: Sample implementation with per-instance locking

Test Plan

CassandraConfigurationOverlayTest: object validation, yaml and jvm opts merge
ConfigurationOverlaySnapshotTest: hash determinism, caching, format
InMemoryConfigurationProviderTest: store/get, optimistic concurrency, instance isolation, concurrent access

isaacreath · 2026-05-18T13:35:41Z

+    public CassandraConfigurationOverlay updated(@Nullable Map<String, JsonNode> cassandraYamlUpdates,
+                                                 @Nullable List<String> addJvmOpts,
+                                                 @Nullable List<String> removeJvmOpts)


Right now this function provides differing approaches for updating values between cassandraYaml and extraJvmOpts.

For cassandraYaml, if I want to update an existing override, I pass the new value in through the cassandraYamlUpdates map.

For extraJvmpOpts, I need to add the existing JVM option to removeJvmOpts and the new value to addJvmOpts.

Is there any way to offer a similar approach for both so that the semantics are consistent for callers?

Changed extraJvmOpts from List to Map<String, String> to provide a consistent update API between cassandraYaml and extraJvmOpts. Both now follow the same semantics in the updated() method: pass a key with a non-null value to upsert, or a null value to remove. This eliminates the need for separate addJvmOpts/removeJvmOpts parameters. Keys use the full JVM flag format (e.g. -Dcassandra.jmx.local.port -> 7199). See commit 0f17ae7.

isaacreath · 2026-05-18T14:01:28Z

+    @NotNull
+    public JsonNode cassandraYaml()
+    {
+        return cassandraYaml;


It's technically possible to mutate cassandraYaml by accessing the value from here, casting it to a ObjectNode, then using ObjectNode#put to update a key.

To preserve the immutable property of this object, we may want to consider returning a copy of the cassandraYaml. That said, this could create a lot of unnecessary objects if the immutability properties aren't necessary.

Since this is an internal API, I opted to avoid defensive copies on every accessor call to minimize unnecessary object creation. I updated the constructor to deep-copy the input ObjectNode to prevent external mutation through the original reference, and updated the accessor Javadoc to mention that callers must not mutate the returned node (use updated() instead). This gives us immutability guarantees at construction time without the overhead of copying on every read. See commit e95958e.

isaacreath · 2026-05-18T19:51:10Z

+        node.put("hash", hash());
+        node.put("lastModified", lastModified.toString());
+        node.set("configuration", MAPPER.valueToTree(configuration));
+        return node.toString();


Nit: Do we want to consider using a pretty print string here? (i.e. toPrettyString()). Could make debugging easier.

Addressed on 7f8a81a

isaacreath · 2026-05-18T19:59:21Z

+     * @return the configuration overlay snapshot, or {@code null} if no overlay exists for the instance
+     */
+    @Nullable
+    ConfigurationOverlaySnapshot getConfiguration(InstanceMetadata instance);


Did you consider getOverlay / storeOverlay instead of getConfiguration / storeConfiguration? The use of overlay can indicate to the reader of callers of this interface that we are not getting a fully materialized config, but it may not be a big deal.

good call, updated on f651dd8

isaacreath · 2026-05-19T14:51:26Z

+        CassandraConfigurationOverlay overlay1 = new CassandraConfigurationOverlay(yaml1, Arrays.asList("-Xmx4G"));
+        CassandraConfigurationOverlay overlay2 = new CassandraConfigurationOverlay(yaml2, Arrays.asList("-Xmx4G"));


Looks like CI is failing here. Needs an import? import java.util.Arrays

isaacreath · 2026-05-19T20:10:11Z

+1 (nb)

frankgh

I think we should try to use vertx JSON framework here. I provided a sample of what this would look like here: 23ba281

frankgh · 2026-05-19T22:13:33Z

+ */
+public class CassandraConfigurationOverlay
+{
+    private static final ObjectMapper MAPPER = new ObjectMapper();


ideally we should try to use the framework's JSON functionality to avoid allocating additional resources (for JSON processing) etc.

Good call, this is more fluent and simplify things. The reason I used jackson is to perform json schema validation in the future but we can convert from vertx json to jackson vertx if needed in the future.

frankgh · 2026-05-20T00:08:08Z

+        Object lock = locks.computeIfAbsent(instance.id(), k -> new Object());
+        synchronized (lock)
+        {
+            ConfigurationOverlaySnapshot current = overlays.get(instance.id());
+
+            if (current == null && originalHash != null)
+            {
+                return false;
+            }
+
+            if (current != null && (originalHash == null || !current.hash().equals(originalHash)))
+            {
+                return false;
+            }
+
+            overlays.put(instance.id(), newSnapshot);
+            return true;
+        }


do we really need locks here? Isn't the concurrent hashmap already a DS that supports this natively?

Suggested change

Object lock = locks.computeIfAbsent(instance.id(), k -> new Object());

synchronized (lock)

{

ConfigurationOverlaySnapshot current = overlays.get(instance.id());

if (current == null && originalHash != null)

{

return false;

}

if (current != null && (originalHash == null || !current.hash().equals(originalHash)))

{

return false;

}

overlays.put(instance.id(), newSnapshot);

return true;

}

return overlays.compute(instance.id(), (k, current) -> {

if (current == null && originalHash != null)

{

return null;

}

if (current != null && (originalHash == null || !current.hash().equals(originalHash)))

{

return current;

}

return newSnapshot;

}) == newSnapshot;

The reason I used an explicit lock is to prevent contention within the CHM bucket but I guess this is premature optimization so I'm ok to remove and rely only on CHM.

pauloricardomg · 2026-05-20T20:14:48Z

@frankgh incorporated your feedback on 7ce2732, but also reverted a previous change to have extraJvmOptions as a map instead of a list (15673bd) and added a new commit ad56510 to validate conflicting jvm boolean options are rejected during merge.

frankgh

+1 looks good. I think we need to rebase and ensure CI is green

Introduce the pluggable overlay storage abstraction as defined in CEP-62 with ConfigurationProvider interface, CassandraConfigurationOverlay model, ConfigurationOverlaySnapshot wrapper, and InMemoryConfigurationProvider sample implementation.

… String> Make update semantics consistent between cassandraYaml and extraJvmOpts: both now use a map where a null value removes the entry and a non-null value upserts it. Keys use the full JVM flag (e.g. -Dcassandra.jmx.local.port).

…mmutability The constructor now deep-copies the input ObjectNode so that external mutation of the original reference cannot corrupt the overlay. The accessor documents that callers must not mutate the returned node.

Improves readability of CassandraConfigurationOverlay and ConfigurationOverlaySnapshot string representations in logs and debugging output.

…rlay/storeOverlay

… String> extraJvmOpts

…laySnapshot

…<String, String>" This reverts commit 53b2108.

… String> Make update semantics consistent between cassandraYaml and extraJvmOpts: both now use a map where a null value removes the entry and a non-null value upserts it. Keys use the full JVM flag (e.g. -Dcassandra.jmx.local.port).

…y merge Detect and reject conflicting -XX:+Option / -XX:-Option pairs after merging extraJvmOpts updates. Users can still replace one form with the other by removing the old key in the same update.

frankgh self-requested a review May 18, 2026 16:45

isaacreath reviewed May 18, 2026

View reviewed changes

pauloricardomg force-pushed the worktree-CASSSIDECAR-424 branch from 9ab8bb4 to f651dd8 Compare May 18, 2026 21:23

isaacreath reviewed May 19, 2026

View reviewed changes

pauloricardomg force-pushed the worktree-CASSSIDECAR-424 branch 2 times, most recently from a45830f to bd32867 Compare May 19, 2026 20:02

frankgh reviewed May 20, 2026

View reviewed changes

pauloricardomg force-pushed the worktree-CASSSIDECAR-424 branch from bd32867 to ad56510 Compare May 20, 2026 20:08

frankgh approved these changes May 21, 2026

View reviewed changes

pauloricardomg and others added 11 commits May 21, 2026 14:27

CASSSIDECAR-424: Use toPrettyString() in toString() for overlay classes

903a712

Improves readability of CassandraConfigurationOverlay and ConfigurationOverlaySnapshot string representations in logs and debugging output.

CASSSIDECAR-424: Rename getConfiguration/storeConfiguration to getOve…

b48388b

…rlay/storeOverlay

CASSSIDECAR-424: Fix ConfigurationOverlaySnapshotTest for Map<String,…

1c3ef86

… String> extraJvmOpts

CASSSIDECAR-424: Rename configuration to overlay in ConfigurationOver…

004e8e5

…laySnapshot

Revert "CASSSIDECAR-424: Change extraJvmOpts from List<String> to Map…

ce4ab09

…<String, String>" This reverts commit 53b2108.

review CASSSIDECAR-424

aa8f1ec

CASSSIDECAR-424: Reject conflicting -XX boolean JVM options in overla…

c7b8446

…y merge Detect and reject conflicting -XX:+Option / -XX:-Option pairs after merging extraJvmOpts updates. Users can still replace one form with the other by removing the old key in the same update.

pauloricardomg force-pushed the worktree-CASSSIDECAR-424 branch from ad56510 to c7b8446 Compare May 21, 2026 18:32

		CassandraConfigurationOverlay overlay1 = new CassandraConfigurationOverlay(yaml1, Arrays.asList("-Xmx4G"));
		CassandraConfigurationOverlay overlay2 = new CassandraConfigurationOverlay(yaml2, Arrays.asList("-Xmx4G"));

Conversation

pauloricardomg commented May 15, 2026

Summary

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloricardomg May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacreath commented May 19, 2026

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauloricardomg commented May 20, 2026

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pauloricardomg May 18, 2026 •

edited

Loading