nvharikrishna commented on code in PR #312:
URL: https://github.com/apache/cassandra-sidecar/pull/312#discussion_r2763956393
##########
server/src/test/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImplTest.java:
##########
@@ -200,6 +200,56 @@ void testResolvesDefaultDirectories()
assertThat(metadata.savedCachesDir()).isEqualTo(rootDir +
"/saved_caches");
}
+ @Test
+ void testDefaultStoragePort()
+ {
+ String rootDir = tempDir.toString();
+ InstanceMetadata metadata = InstanceMetadataImpl.builder()
+ .id(ID)
+ .host(HOST)
+ .port(PORT)
+
.metricRegistry(METRIC_REGISTRY)
+ .storageDir(rootDir)
+ .build();
+ assertThat(metadata.storagePort()).isEqualTo(7000);
+ }
+
+ @Test
+ void testCustomStoragePort()
+ {
+ String rootDir = tempDir.toString();
+ for (int customStoragePort : new int[]{ 1, 65535, 7005 })
+ {
+ InstanceMetadata metadata = InstanceMetadataImpl.builder()
+ .id(ID)
+ .host(HOST)
+ .port(PORT)
+
.storagePort(customStoragePort)
+
.metricRegistry(METRIC_REGISTRY)
+
.storageDir(rootDir)
+ .build();
+ assertThat(metadata.storagePort()).isEqualTo(customStoragePort);
+ }
+ }
+
+ @Test
+ void testInvalidStoragePort()
Review Comment:
done
##########
server/src/test/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImplTest.java:
##########
@@ -200,6 +200,56 @@ void testResolvesDefaultDirectories()
assertThat(metadata.savedCachesDir()).isEqualTo(rootDir +
"/saved_caches");
}
+ @Test
+ void testDefaultStoragePort()
+ {
+ String rootDir = tempDir.toString();
+ InstanceMetadata metadata = InstanceMetadataImpl.builder()
+ .id(ID)
+ .host(HOST)
+ .port(PORT)
+
.metricRegistry(METRIC_REGISTRY)
+ .storageDir(rootDir)
+ .build();
+ assertThat(metadata.storagePort()).isEqualTo(7000);
+ }
+
+ @Test
+ void testCustomStoragePort()
+ {
+ String rootDir = tempDir.toString();
+ for (int customStoragePort : new int[]{ 1, 65535, 7005 })
+ {
Review Comment:
done
##########
server/src/test/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImplTest.java:
##########
@@ -200,6 +200,56 @@ void testResolvesDefaultDirectories()
assertThat(metadata.savedCachesDir()).isEqualTo(rootDir +
"/saved_caches");
}
+ @Test
+ void testDefaultStoragePort()
+ {
+ String rootDir = tempDir.toString();
+ InstanceMetadata metadata = InstanceMetadataImpl.builder()
+ .id(ID)
+ .host(HOST)
+ .port(PORT)
+
.metricRegistry(METRIC_REGISTRY)
+ .storageDir(rootDir)
+ .build();
+ assertThat(metadata.storagePort()).isEqualTo(7000);
+ }
+
+ @Test
+ void testCustomStoragePort()
Review Comment:
done
##########
server/src/test/java/org/apache/cassandra/sidecar/config/SidecarConfigurationTest.java:
##########
@@ -588,6 +588,41 @@ void testRepairConfiguration() throws IOException
assertThat(repairConfig.repairPollInterval()).isEqualTo(MillisecondBoundConfiguration.parse("500ms"));
}
+ @Test
+ void testStoragePortConfiguration() throws IOException
+ {
+ // Test with explicit storage_port
+ String yamlWithStoragePort = "cassandra_instances:\n" +
+ " - id: 1\n" +
+ " host: localhost1\n" +
+ " port: 9042\n" +
+ " storage_port: 7001\n" +
+ " storage_dir: /var/lib/cassandra\n" +
+ " staging_dir:
/var/lib/cassandra/staging\n" +
+ " jmx_host: 127.0.0.1\n" +
+ " jmx_port: 7199\n" +
+ " jmx_ssl_enabled: false";
+ SidecarConfigurationImpl configWithStoragePort =
SidecarConfigurationImpl.fromYamlString(yamlWithStoragePort);
+
assertThat(configWithStoragePort.cassandraInstances()).isNotNull().hasSize(1);
+ InstanceConfiguration instance1 =
configWithStoragePort.cassandraInstances().get(0);
+ assertThat(instance1.storagePort()).isEqualTo(7001);
+
+ // Test without storage_port (should return null)
+ String yamlWithoutStoragePort = "cassandra_instances:\n" +
Review Comment:
done
##########
server/src/main/java/org/apache/cassandra/sidecar/cluster/instance/InstanceMetadataImpl.java:
##########
@@ -237,6 +254,7 @@ public static class Builder implements
DataObjectBuilder<Builder, InstanceMetada
protected String host;
protected String ipAddress;
protected int port;
+ protected int storagePort = DEFAULT_STORAGE_PORT;
Review Comment:
Thanks for the thoughtful review! Let me share the reasoning behind using
DEFAULT_STORAGE_PORT:
1. Independence from Cassandra process availability
Goal is to let sidecar function even when Cassandra process is not running
or unreachable. This is particularly important in scenarios where:
- Sidecar is started before Cassandra
- Cassandra is temporarily unavailable
- During initialization or live migration (CEP-40) when Cassandra may not
yet be part of the ring
2. 7000 port - following Cassandra's default value
The DEFAULT_STORAGE_PORT = 7000 is not Sidecar's own local value but rather
the default value that Cassandra itself uses (defined in cassandra.yaml and
Config.java). By using this default, it is mirroring Cassandra's own behavior
when the value isn't explicitly configured. This should actually help
developers since it matches Cassandra's defaults. Of course, if an operator has
configured a non-standard storage_port in their Cassandra instance, they should
configure the same value in Sidecar's configuration.
Cassandra uses IP + port for node identification, and Sidecar needs to
correctly identify nodes even when Cassandra is temporarily unreachable. Using
null, -1, or error messages when we can't reach Cassandra would actually hinder
operations in the above scenarios. I feel the default value simply provides a
sensible fallback.
Does this reasoning make sense?
--
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]
