CASSSIDECAR-427: Implement basic configuration retrieval in ConfigurationManager by pauloricardomg · Pull Request #358 · apache/cassandra-sidecar

pauloricardomg · 2026-05-29T16:49:22Z

Summary

Implement the basic GET configuration flow in the Configuration Manager. When handling retrieval requests, the manager fetches the current overlay from the ConfigurationProvider, merges it with the base template to compute the effective configuration, and returns the result with its SHA-256 hash and last modified timestamp.

New classes

ConfigurationManager: Retrieves effective configuration by loading the base template via ConfigUtils.loadConfiguration and merging it with the provider overlay via ConfigurationOverlaySnapshot.overlay(). Wraps provider failures in ConfigurationManagerException.
ConfigurationManagerException: Unchecked exception wrapping provider failures with a descriptive message.
ConfigUtils: Utilities for YAML loading (loadYaml), deep merge of configurations (mergeConfigurations), and loading a YAML file as a ConfigurationOverlaySnapshot (loadConfiguration).

Changes to existing classes

ConfigurationOverlaySnapshot: Adds an overlay() method that produces a new snapshot by deep-merging another snapshot on top, combining cassandraYaml recursively, merging extraJvmOpts, and using the max of both lastModified timestamps. Renames overlay field/accessor to configuration to better reflect its role when representing the effective (merged) configuration.

Test plan

ConfigurationManagerTest: Verifies effective configuration retrieval with no overlay, with overlay (merge semantics), and provider failure propagation
ConfigUtilsTest: Verifies YAML loading, deep merge logic (overlay wins, new keys, nested objects, empty overlay), and loadConfiguration snapshot creation
ConfigurationOverlaySnapshotTest: Verifies overlay() merge of yaml keys, JVM opts, max lastModified, and deep nested object merging
FileBasedConfigurationProviderTest: Updated to use renamed configuration() accessor
Run ./gradlew :server:test --tests "org.apache.cassandra.sidecar.configmanagement.*" - all 50 tests pass

isaacreath · 2026-06-05T20:27:32Z

+                                                                other.configuration().cassandraYaml());
+
+        Map<String, String> mergedOpts = new LinkedHashMap<>(configuration.extraJvmOpts());
+        mergedOpts.putAll(other.configuration().extraJvmOpts());


This will add new keys for any key which is not present in the mergedOpts map. Is that the behavior we want to expose? Or do we want to limit overlays to known keys in the base template?

Either way worth explicitly documenting in the javadoc.

overlays can add keys not present in the base template, added it to javadoc on 7c81cc8

isaacreath · 2026-06-05T20:31:43Z

+        Map<String, String> mergedOpts = new LinkedHashMap<>(configuration.extraJvmOpts());
+        mergedOpts.putAll(other.configuration().extraJvmOpts());
+
+        Instant mergedLastModified = lastModified.isAfter(other.lastModified)


Since the merge produces a new ConfigurationOverlaySnapshot, shouldn't lastModified be Instant.now()? If not, what are the intended semantics of lastModified?

The javadoc in https://github.com/apache/cassandra-sidecar/pull/358/changes#diff-e190c28aea1ad1a802b4cbcdadc180917c653b3b9aac34b0eb23eb4c680e277bR48-R58 helps clarify this a bit.

Just overlaying the configuration over a base template does not produce a new configuration. last modified is the maximum modification timestamp of the base template and the overlay. lastModified is only instant.now when updating the overlay or the base template by modifying the file on disk. Added clarification to javadoc on 99072c3

isaacreath · 2026-06-05T20:37:57Z

+
+        JsonObject result = base.copy();
+        JsonObject overlayCopy = overlay.copy();
+        for (Map.Entry<String, Object> field : overlayCopy)


Similar question as above about the intended semantics when a overlay contains keys not in the base configuration.

clarified on 7c81cc8

skoppu22 · 2026-06-08T15:59:22Z

+            }
+            else
+            {
+                result.put(fieldName, overlayValue);


Looks like if base value for a key is not null and overlay value for the same is null, this line still picks overlay value null. Is this expected? We are not doing this for JsonObjects as line 114 above picks base copy. But we are doing this differently for individual keys.

My assumption is overlay contains only overridden keys, while base contains all. For example,

If base is
a: 'a'
b: 'b'
c: 'c'

Then if overlay is
b:'b2'

Does it not mean admins want to only override b, while picking base value for others? In that case, the line 136 missing this case right?

Add null handling on ad29f2c and also added test case to ensure overlay will be correct for that case.

skoppu22 · 2026-06-08T16:08:47Z

+        Instant lastModified;
+        try
+        {
+            lastModified = Files.getLastModifiedTime(yamlPath).toInstant();


There is no synchronization to avoid race condition between loading yaml at line 65 and retrieving last modified time at line 69. There is a possibility of associating different modified time for older yaml if the file was modified in between.

As we are reading from disk,

either we need to read both contents and timestamp in a single call if such an API exists

or do optimistic synchronization
t1 = read modified time
data = read contents
t2 = read modified time
if t2 != t1, file was modified and repeat the action.

good catch, added optimistic concurrency on 031b999

skoppu22 · 2026-06-08T16:13:12Z

+                                                                other.configuration().cassandraYaml());
+
+        Map<String, String> mergedOpts = new LinkedHashMap<>(configuration.extraJvmOpts());
+        mergedOpts.putAll(other.configuration().extraJvmOpts());


Don't we need to call validateNoConflictingBooleanOpts on these merged JVM opts?

Makes sense, updated 6ec34b8 to prefer base if there's a conflicting boolean jvm opt on overlay.

skoppu22 · 2026-06-08T16:16:57Z

    public JsonObject toJson()
    {
        return new JsonObject()
+               .put("hash", hash())


fromJson doesn't have "hash", while toJson having it

Yes, that's expected. hash is computed from the configuration contents and cached — it's not stored state. toJson() includes it for observability (e.g., logging, API responses), but fromJson() correctly skips it since it will be recomputed on demand from the deserialized configuration.

skoppu22 · 2026-06-08T16:19:49Z

+    @NotNull
+    public ConfigurationOverlaySnapshot getEffectiveConfiguration(InstanceMetadata instance)
+    {
+        ConfigurationOverlaySnapshot baseSnapshot = ConfigUtils.loadConfiguration(baseTemplatePath);


Do we plan to cache base snapshot in upcoming PRs, as it doesn't change frequently, and we are involving IO every time here.

Added base template caching on b0125eb

skoppu22 · 2026-06-08T16:21:17Z

+        }
+
+        JsonObject result = base.copy();
+        JsonObject overlayCopy = overlay.copy();


Why do we need to make a copy of overlay when we are not modifying it?

not needed, removed on f62ab37

pauloricardomg · 2026-06-10T18:09:06Z

@isaacreath @skoppu22 thanks for the helfpul comments, addressed them

skoppu22 · 2026-06-11T11:33:19Z

+        throw new UnsupportedOperationException();
+    }
+
+    /**


Looks like older JavaDoc left over here

Fixed on 23a3ed8

skoppu22 · 2026-06-11T11:37:56Z

+            String key = entry.getKey();
+            if (CassandraConfigurationOverlay.hasConflictingBooleanOpt(mergedOpts, key))
+            {
+                continue;


Are we preferring base option if there is a conflict? Either we need to log a warning or throw an error. And javadoc says 'merges', doesn't mention that we are picking base option here on conflict. It can be confusing for the users, better to callout whatever the approach we pick here.

Normally it shouldn't be possible to add conflicting option, since we validate that when updating configs. However in case the remote provider adds a conflicting option it's better to ignore and log a warning then fail, otherwise this can prevent Cassandra from starting. I logged a warning and updated javadoc on f95be70

skoppu22 · 2026-06-12T13:15:56Z

+
+    /**
+     * Deep-merges the overlay onto the base configuration. For nested objects both base and overlay
+     * contain, fields are merged recursively. For all other node types (scalars, arrays, nulls),


[Minor] 'nulls' can be removed

Good catch, fixed on c9d81a5. Thanks for the review 🙏

isaacreath · 2026-06-15T19:59:41Z

+1 (nb).

… retrieval Introduce ConfigurationManager that computes the effective configuration for a Cassandra instance by fetching the overlay from the ConfigurationProvider and merging it with the base cassandra.yaml template. New classes: - ConfigurationManager: retrieves effective configuration by loading the base template via ConfigUtils.loadConfiguration and merging it with the provider overlay via ConfigurationOverlaySnapshot.overlay() - ConfigurationManagerException: wraps provider failures with a descriptive message - ConfigUtils: utilities for YAML loading (loadYaml), deep merge of configurations (mergeConfigurations), and loading a YAML file as a ConfigurationOverlaySnapshot (loadConfiguration) ConfigurationOverlaySnapshot gains an overlay() method that produces a new snapshot by deep-merging another snapshot on top, combining cassandraYaml recursively, merging extraJvmOpts, and using the max of both lastModified timestamps.

…t in the base configuration

…ion keys

…ions

…on during load

…ring merge

…le is unmodified

…olean JVM opts during overlay merge

…pping behavior

pauloricardomg force-pushed the CASSSIDECAR-427 branch 2 times, most recently from e18a529 to 28ccef4 Compare June 1, 2026 20:47

isaacreath reviewed Jun 5, 2026

View reviewed changes

skoppu22 reviewed Jun 8, 2026

View reviewed changes

pauloricardomg force-pushed the CASSSIDECAR-427 branch from 79da900 to e98459d Compare June 10, 2026 17:59

skoppu22 reviewed Jun 11, 2026

View reviewed changes

pauloricardomg force-pushed the CASSSIDECAR-427 branch from e98459d to f95be70 Compare June 11, 2026 17:12

skoppu22 reviewed Jun 12, 2026

View reviewed changes

skoppu22 approved these changes Jun 12, 2026

View reviewed changes

pauloricardomg force-pushed the CASSSIDECAR-427 branch from f95be70 to c9d81a5 Compare June 12, 2026 14:42

isaacreath approved these changes Jun 15, 2026

View reviewed changes

pauloricardomg force-pushed the CASSSIDECAR-427 branch from c9d81a5 to 6f68038 Compare June 16, 2026 14:13

pauloricardomg added 10 commits June 17, 2026 12:19

CASSSIDECAR-427: Document that overlays can introduce keys not presen…

b2d9ed1

…t in the base configuration

CASSSIDECAR-427: Document lastModified semantics for merged snapshots

a9875ce

CASSSIDECAR-427: Skip null overlay values to preserve base configurat…

5932f86

…ion keys

CASSSIDECAR-427: Require non-null base and overlay in mergeConfigurat…

952dcc2

…ions

CASSSIDECAR-427: Add optimistic concurrency check for file modificati…

f4d72f8

…on during load

CASSSIDECAR-427: Prefer base value on conflicting boolean JVM opts du…

7ecc628

…ring merge

CASSSIDECAR-427: Remove unnecessary overlay copy in mergeConfigurations

91ede66

CASSSIDECAR-427: Cache base template snapshot and skip reload when fi…

566f98d

…le is unmodified

CASSSIDECAR-427: Fix import order in ConfigUtils

cdb22e6

pauloricardomg added 3 commits June 17, 2026 12:19

CASSSIDECAR-427: Remove stale javadoc from ConfigUtils.loadConfiguration

549339b

CASSSIDECAR-427: Log warning and document behavior for conflicting bo…

ec02c2f

…olean JVM opts during overlay merge

CASSSIDECAR-427: Fix mergeConfigurations javadoc to document null-ski…

827a8cd

…pping behavior

pauloricardomg force-pushed the CASSSIDECAR-427 branch from 6f68038 to 827a8cd Compare June 17, 2026 16:19

Conversation

pauloricardomg commented May 29, 2026

Summary

New classes

Changes to existing classes

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!

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!

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 commented Jun 10, 2026

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 Jun 11, 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!

Choose a reason for hiding this comment

Uh oh!

isaacreath commented Jun 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pauloricardomg Jun 11, 2026 •

edited

Loading