CASSSIDECAR-179: Added additional dirs required for live migration #163

nvharikrishna · 2024-12-13T11:19:03Z

Making this change as part of CEP-40.

Adding commitlog, hints, saved caches and local system data file directories to InstanceMetadata so that they migrated as part of CEP-40.

For CASSSIDECAR-179

bbotella

Just a couple of nits. Otherwise, LGTM.

Thanks for the patch!
+1 (nb)

bbotella · 2024-12-13T22:36:47Z

server/src/main/java/org/apache/cassandra/sidecar/config/yaml/InstanceConfigurationImpl.java

@@ -92,9 +112,13 @@ public InstanceConfigurationImpl(int id,
        this.id = id;
        this.host = host;
        this.port = port;
-        this.dataDirs = Collections.unmodifiableList(dataDirs);
+        this.dataDirs = dataDirs == null ? null : Collections.unmodifiableList(dataDirs);


This looks like an unrelated bug fix?

Not trying to fix anything actually :). Trying to initialize the member variables in a single constructor. Earlier constructor initialized dataDirs with null, but this constructor is wrapping dataDirs with Collections.unmodifiableList which doesn't accept null. So checking for null before wrapping it.

when would data dirs be null (or empty)? Maybe this needs to be validated instead of accepting null

when would data dirs be null (or empty)? Maybe this needs to be validated instead of accepting null

It is set to null if the data_dirs is not configured in sidecar.yaml. I was not sure why it was allowed to be null earlier. So did not change the behavior. I can make changes now though.

bbotella · 2024-12-13T22:37:39Z

...er/src/test/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImplTest.java

@@ -39,6 +39,10 @@ class InstanceMetadataImplTest
    private static final String DATA_DIR_2 = "test/data/data2";
    private static final String CDC_DIR = "cdc_dir";
    private static final String STAGING_DIR = "staging_dir";
+    private static final String COMMITLOG_DIR = "commitlog";


Nit: Should we append _dir to keep consistency? (both cdc and staging append it)

I have used the names used in cassandra.yaml for newly introduced directories. Wanted to use default names used in Cassandra, but I don't mind changing the names if required.

we should not, this is the default directory name for the commit log in Cassandra, so it would be incorrect to add _dir. See https://github.com/apache/cassandra/blob/trunk/conf/cassandra.yaml#L411

so it would be incorrect to add _dir.

Wondering why CDC_DIR has "_dir" postfix. As per cassandra.yaml, cdc_raw_directory is cdc_raw.

maybe because it's test code, but it feels we should align with Cassandra directories

I ended up with setting these values to non default values in order to test that InstanceMetaImpl is not using default values when these directories are set explicitly. Using "_dir" like in previous constants.

(I wrote this test class a few days back with the same intention, could not recollect it earlier, my apologies!)

nvharikrishna

@bbotella Thanks for the review. Addressed the comments.

nvharikrishna · 2024-12-18T13:53:17Z

server/src/main/java/org/apache/cassandra/sidecar/config/yaml/InstanceConfigurationImpl.java

@@ -92,9 +112,13 @@ public InstanceConfigurationImpl(int id,
        this.id = id;
        this.host = host;
        this.port = port;
-        this.dataDirs = Collections.unmodifiableList(dataDirs);
+        this.dataDirs = dataDirs == null ? null : Collections.unmodifiableList(dataDirs);


Not trying to fix anything actually :). Trying to initialize the member variables in a single constructor. Earlier constructor initialized dataDirs with null, but this constructor is wrapping dataDirs with Collections.unmodifiableList which doesn't accept null. So checking for null before wrapping it.

nvharikrishna · 2024-12-18T14:00:18Z

...er/src/test/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImplTest.java

@@ -39,6 +39,10 @@ class InstanceMetadataImplTest
    private static final String DATA_DIR_2 = "test/data/data2";
    private static final String CDC_DIR = "cdc_dir";
    private static final String STAGING_DIR = "staging_dir";
+    private static final String COMMITLOG_DIR = "commitlog";


I have used the names used in cassandra.yaml for newly introduced directories. Wanted to use default names used in Cassandra, but I don't mind changing the names if required.

sarankk · 2024-12-19T22:23:01Z

server/src/main/java/org/apache/cassandra/sidecar/config/yaml/InstanceConfigurationImpl.java

+             null,
+             null,
+             null,
+             null,


For commit log, hints and saved caches, can we build it if it is not set. Like we do in Cassandra, by taking in CASSANDRA_HOME

I agree that we should simplify Sidecar configuration, where we follow Cassandra's configuration. Ideally, we will only have to configure the cassandra_home configuration per instance. And then we resolve the directory files in Cassandra unless explicitly provided.

frankgh

I have some suggestions to simplify configuration and make operability simpler

frankgh · 2024-12-20T19:44:45Z

server/src/main/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadata.java

Just a general comment. We should update the sidecar.yaml configuration file under conf/sidecar.yaml with configuration parameters. Ideally I think we want to only have to specify the cassandra_home_dir , and optionally specify Cassandra directories. The exception are data directories

Ideally I think we want to only have to specify the cassandra_home_dir , and optionally specify Cassandra directories. The exception are data directories

As per the documentation comment in cassandra.yaml, the default data directory is$CASSANDRA_HOME/data/data. Is there any reason to take exception for data directories?

Followed the footsteps of existing code. I would be happy to adopt cassandra_home_dir as part of this pr.

Patch by Venkata Harikrishna Nukala; reviewed by <TBD> for CASSANDRASC-179

nvharikrishna · 2024-12-23T16:04:10Z

server/src/main/java/org/apache/cassandra/sidecar/config/yaml/InstanceConfigurationImpl.java

    protected final String jmxRolePassword;

-    public InstanceConfigurationImpl()


Removed this in order to reduce the confusion with what can be null and what cannot be null.

nvharikrishna · 2024-12-23T16:04:29Z

server/src/main/java/org/apache/cassandra/sidecar/config/yaml/InstanceConfigurationImpl.java


 /**
 * Encapsulates the basic configuration needed to connect to a single Cassandra instance
 */
 public class InstanceConfigurationImpl implements InstanceConfiguration
 {
-    @JsonProperty("id")


Using constructor for this.

nvharikrishna · 2024-12-23T16:05:59Z

server/src/main/java/org/apache/cassandra/sidecar/config/yaml/InstanceConfigurationImpl.java

-                                     String jmxRolePassword)
+    @JsonCreator
+    public InstanceConfigurationImpl(@JsonProperty("id") int id,
+                                     @NotNull @JsonProperty("host") String host,


@frankgh I guessing a bit for NotNull and Nullable of these fields. Please suggest changes if my interpretation is incorrect.

nvharikrishna · 2024-12-23T16:46:09Z

server/src/main/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImpl.java

@@ -52,15 +65,42 @@ protected InstanceMetadataImpl(Builder builder)
        id = builder.id;
        host = builder.host;
        port = builder.port;
+        cassandraHomeDir = FileUtils.maybeResolveHomeDirectory(builder.cassandraHomeDir);
+        Path cassandraHomeDirPath = cassandraHomeDir == null ?  null : Paths.get(cassandraHomeDir);


Okay with having cassandraHomeDir as a mandatory configuration? If yes, then I can push changes to make it mandatory. Assuming it is not mandatory for now.

bbotella reviewed Dec 13, 2024

View reviewed changes

nvharikrishna commented Dec 18, 2024

View reviewed changes

sarankk reviewed Dec 19, 2024

View reviewed changes

frankgh reviewed Dec 20, 2024

View reviewed changes

CASSSIDECAR-179: Added additional dirs required for live migration

4b4f1e6

Patch by Venkata Harikrishna Nukala; reviewed by <TBD> for CASSANDRASC-179

nvharikrishna force-pushed the 179-instace-config-additional-dirs branch 2 times, most recently from 172239a to cc203a3 Compare December 23, 2024 16:21

Addressed review comments.

b698822

nvharikrishna force-pushed the 179-instace-config-additional-dirs branch from cc203a3 to b698822 Compare December 23, 2024 16:32

nvharikrishna commented Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSSIDECAR-179: Added additional dirs required for live migration #163

CASSSIDECAR-179: Added additional dirs required for live migration #163

nvharikrishna commented Dec 13, 2024

bbotella left a comment

bbotella Dec 13, 2024

nvharikrishna Dec 18, 2024

frankgh Dec 20, 2024

nvharikrishna Dec 23, 2024

bbotella Dec 13, 2024

nvharikrishna Dec 18, 2024

frankgh Dec 20, 2024

nvharikrishna Dec 23, 2024

frankgh Dec 23, 2024

nvharikrishna Dec 23, 2024

nvharikrishna left a comment

nvharikrishna Dec 18, 2024

nvharikrishna Dec 18, 2024

sarankk Dec 19, 2024

frankgh Dec 20, 2024

frankgh left a comment

frankgh Dec 20, 2024

nvharikrishna Dec 23, 2024

nvharikrishna Dec 23, 2024

nvharikrishna Dec 23, 2024

nvharikrishna Dec 23, 2024

nvharikrishna Dec 23, 2024

nvharikrishna Dec 23, 2024

		protected final String jmxRolePassword;

		public InstanceConfigurationImpl()

CASSSIDECAR-179: Added additional dirs required for live migration #163

Are you sure you want to change the base?

CASSSIDECAR-179: Added additional dirs required for live migration #163

Conversation

nvharikrishna commented Dec 13, 2024

bbotella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvharikrishna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment