isaacreath commented on code in PR #358:
URL: https://github.com/apache/cassandra-sidecar/pull/358#discussion_r3365194863
##########
server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationOverlaySnapshot.java:
##########
@@ -21,34 +21,64 @@
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.time.Instant;
+import java.util.LinkedHashMap;
+import java.util.Map;
import java.util.Objects;
import io.vertx.core.json.JsonObject;
import org.jetbrains.annotations.NotNull;
/**
- * Represents a snapshot of a configuration overlay with its metadata.
- * The SHA-256 hash is dynamically computed from the overlay contents and
cached.
+ * Represents a snapshot of a configuration with its metadata.
+ * The SHA-256 hash is dynamically computed from the configuration contents
and cached.
*/
public class ConfigurationOverlaySnapshot
{
@NotNull
private final Instant lastModified;
@NotNull
- private final CassandraConfigurationOverlay overlay;
+ private final CassandraConfigurationOverlay configuration;
private volatile String hash;
public ConfigurationOverlaySnapshot(@NotNull Instant lastModified,
- @NotNull CassandraConfigurationOverlay
overlay)
+ @NotNull CassandraConfigurationOverlay
configuration)
{
this.lastModified = Objects.requireNonNull(lastModified, "lastModified
must not be null");
- this.overlay = Objects.requireNonNull(overlay, "overlay must not be
null");
+ this.configuration = Objects.requireNonNull(configuration,
"configuration must not be null");
}
/**
- * Returns the SHA-256 hash of the overlay contents, prefixed with
"sha256:".
+ * Merges another snapshot on top of this one, producing the effective
configuration.
+ * The other snapshot's values take precedence over this snapshot's values.
+ *
+ * <p>Deep-merges {@code cassandraYaml} (nested objects are recursively
merged, all other
+ * types are replaced by the other snapshot's values). Merges {@code
extraJvmOpts} with the
+ * other snapshot's entries overriding this snapshot's entries on key
conflict.
+ *
+ * @param other the overlay snapshot whose values take precedence
+ * @return a new snapshot with the merged configuration and the max of
both lastModified timestamps
+ */
+ @NotNull
+ public ConfigurationOverlaySnapshot overlay(@NotNull
ConfigurationOverlaySnapshot other)
+ {
+ JsonObject mergedYaml =
ConfigUtils.mergeConfigurations(configuration.cassandraYaml(),
+
other.configuration().cassandraYaml());
+
+ Map<String, String> mergedOpts = new
LinkedHashMap<>(configuration.extraJvmOpts());
+ mergedOpts.putAll(other.configuration().extraJvmOpts());
Review Comment:
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.
##########
server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationOverlaySnapshot.java:
##########
@@ -21,34 +21,64 @@
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.time.Instant;
+import java.util.LinkedHashMap;
+import java.util.Map;
import java.util.Objects;
import io.vertx.core.json.JsonObject;
import org.jetbrains.annotations.NotNull;
/**
- * Represents a snapshot of a configuration overlay with its metadata.
- * The SHA-256 hash is dynamically computed from the overlay contents and
cached.
+ * Represents a snapshot of a configuration with its metadata.
+ * The SHA-256 hash is dynamically computed from the configuration contents
and cached.
*/
public class ConfigurationOverlaySnapshot
{
@NotNull
private final Instant lastModified;
@NotNull
- private final CassandraConfigurationOverlay overlay;
+ private final CassandraConfigurationOverlay configuration;
private volatile String hash;
public ConfigurationOverlaySnapshot(@NotNull Instant lastModified,
- @NotNull CassandraConfigurationOverlay
overlay)
+ @NotNull CassandraConfigurationOverlay
configuration)
{
this.lastModified = Objects.requireNonNull(lastModified, "lastModified
must not be null");
- this.overlay = Objects.requireNonNull(overlay, "overlay must not be
null");
+ this.configuration = Objects.requireNonNull(configuration,
"configuration must not be null");
}
/**
- * Returns the SHA-256 hash of the overlay contents, prefixed with
"sha256:".
+ * Merges another snapshot on top of this one, producing the effective
configuration.
+ * The other snapshot's values take precedence over this snapshot's values.
+ *
+ * <p>Deep-merges {@code cassandraYaml} (nested objects are recursively
merged, all other
+ * types are replaced by the other snapshot's values). Merges {@code
extraJvmOpts} with the
+ * other snapshot's entries overriding this snapshot's entries on key
conflict.
+ *
+ * @param other the overlay snapshot whose values take precedence
+ * @return a new snapshot with the merged configuration and the max of
both lastModified timestamps
+ */
+ @NotNull
+ public ConfigurationOverlaySnapshot overlay(@NotNull
ConfigurationOverlaySnapshot other)
+ {
+ JsonObject mergedYaml =
ConfigUtils.mergeConfigurations(configuration.cassandraYaml(),
+
other.configuration().cassandraYaml());
+
+ Map<String, String> mergedOpts = new
LinkedHashMap<>(configuration.extraJvmOpts());
+ mergedOpts.putAll(other.configuration().extraJvmOpts());
+
+ Instant mergedLastModified = lastModified.isAfter(other.lastModified)
Review Comment:
Since the merge produces a new `ConfigurationOverlaySnapshot`, shouldn't
`lastModified` be `Instant.now()`? If not, what are the intended semantics of
`lastModified`?
##########
server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigUtils.java:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.sidecar.configmanagement;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.time.Instant;
+import java.util.Collections;
+import java.util.Map;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
+import com.fasterxml.jackson.dataformat.yaml.YAMLGenerator;
+import io.vertx.core.json.JsonObject;
+import io.vertx.core.json.jackson.DatabindCodec;
+
+/**
+ * Utility methods for configuration operations: YAML loading and deep merge.
+ */
+public final class ConfigUtils
+{
+ static final YAMLFactory YAML_FACTORY = new YAMLFactory()
+ .disable(YAMLGenerator.Feature.WRITE_DOC_START_MARKER);
+
+ private ConfigUtils()
+ {
+ throw new UnsupportedOperationException();
+ }
+
+ /**
+ * Loads a YAML configuration file and returns it as a {@link
ConfigurationOverlaySnapshot}.
+ * The snapshot's {@code lastModified} is set to the file's modification
time and
+ * {@code extraJvmOpts} is empty.
+ *
+ * <p>If {@code yamlPath} is {@code null}, returns an {@linkplain
ConfigurationOverlaySnapshot#emptySnapshot()
+ * empty snapshot}.
+ *
+ * @param yamlPath path to the YAML configuration file, or {@code null}
for an empty snapshot
+ * @return a snapshot representing the file contents
+ */
+ public static ConfigurationOverlaySnapshot loadConfiguration(Path yamlPath)
+ {
+ if (yamlPath == null)
+ {
+ return ConfigurationOverlaySnapshot.emptySnapshot();
+ }
+ JsonObject yaml = loadYaml(yamlPath);
+ Instant lastModified;
+ try
+ {
+ lastModified = Files.getLastModifiedTime(yamlPath).toInstant();
+ }
+ catch (IOException e)
+ {
+ throw new UncheckedIOException("Failed to read modification time
of " + yamlPath, e);
+ }
+ CassandraConfigurationOverlay overlay = new
CassandraConfigurationOverlay(yaml, Collections.emptyMap());
+ return new ConfigurationOverlaySnapshot(lastModified, overlay);
+ }
+
+ /**
+ * Loads a YAML file into a Vert.x {@link JsonObject}.
+ *
+ * @param yamlPath path to the YAML file
+ * @return the parsed configuration as a JsonObject
+ */
+ @SuppressWarnings("unchecked")
+ public static JsonObject loadYaml(Path yamlPath)
+ {
+ try (JsonParser parser = YAML_FACTORY.createParser(yamlPath.toFile()))
+ {
+ Map<String, Object> map = DatabindCodec.mapper().readValue(parser,
Map.class);
+ return map != null ? new JsonObject(map) : new JsonObject();
+ }
+ catch (IOException e)
+ {
+ throw new UncheckedIOException("Failed to load YAML from " +
yamlPath, e);
+ }
+ }
+
+ /**
+ * 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),
+ * the overlay value replaces the base value. The base node is not
modified.
+ *
+ * @param base the base configuration tree
+ * @param overlay the overlay tree whose values take precedence
+ * @return a new tree with the merged result
+ */
+ public static JsonObject mergeConfigurations(JsonObject base, JsonObject
overlay)
+ {
+ if (base == null)
+ {
+ return overlay.copy();
+ }
+ if (overlay == null || overlay.isEmpty())
+ {
+ return base.copy();
+ }
+
+ JsonObject result = base.copy();
+ JsonObject overlayCopy = overlay.copy();
+ for (Map.Entry<String, Object> field : overlayCopy)
Review Comment:
Similar question as above about the intended semantics when a overlay
contains keys not in the base configuration.
##########
server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationOverlaySnapshot.java:
##########
@@ -21,34 +21,64 @@
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.time.Instant;
+import java.util.LinkedHashMap;
+import java.util.Map;
import java.util.Objects;
import io.vertx.core.json.JsonObject;
import org.jetbrains.annotations.NotNull;
/**
- * Represents a snapshot of a configuration overlay with its metadata.
- * The SHA-256 hash is dynamically computed from the overlay contents and
cached.
+ * Represents a snapshot of a configuration with its metadata.
+ * The SHA-256 hash is dynamically computed from the configuration contents
and cached.
*/
public class ConfigurationOverlaySnapshot
{
@NotNull
private final Instant lastModified;
@NotNull
- private final CassandraConfigurationOverlay overlay;
+ private final CassandraConfigurationOverlay configuration;
private volatile String hash;
public ConfigurationOverlaySnapshot(@NotNull Instant lastModified,
- @NotNull CassandraConfigurationOverlay
overlay)
+ @NotNull CassandraConfigurationOverlay
configuration)
{
this.lastModified = Objects.requireNonNull(lastModified, "lastModified
must not be null");
- this.overlay = Objects.requireNonNull(overlay, "overlay must not be
null");
+ this.configuration = Objects.requireNonNull(configuration,
"configuration must not be null");
}
/**
- * Returns the SHA-256 hash of the overlay contents, prefixed with
"sha256:".
+ * Merges another snapshot on top of this one, producing the effective
configuration.
+ * The other snapshot's values take precedence over this snapshot's values.
+ *
+ * <p>Deep-merges {@code cassandraYaml} (nested objects are recursively
merged, all other
+ * types are replaced by the other snapshot's values). Merges {@code
extraJvmOpts} with the
+ * other snapshot's entries overriding this snapshot's entries on key
conflict.
+ *
+ * @param other the overlay snapshot whose values take precedence
+ * @return a new snapshot with the merged configuration and the max of
both lastModified timestamps
+ */
+ @NotNull
+ public ConfigurationOverlaySnapshot overlay(@NotNull
ConfigurationOverlaySnapshot other)
+ {
+ JsonObject mergedYaml =
ConfigUtils.mergeConfigurations(configuration.cassandraYaml(),
+
other.configuration().cassandraYaml());
+
+ Map<String, String> mergedOpts = new
LinkedHashMap<>(configuration.extraJvmOpts());
+ mergedOpts.putAll(other.configuration().extraJvmOpts());
+
+ Instant mergedLastModified = lastModified.isAfter(other.lastModified)
Review Comment:
The javadoc in
https://github.com/apache/cassandra-sidecar/pull/358/changes#diff-e190c28aea1ad1a802b4cbcdadc180917c653b3b9aac34b0eb23eb4c680e277bR48-R58
helps clarify this a bit.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
