frankgh commented on code in PR #15:
URL:
https://github.com/apache/cassandra-analytics/pull/15#discussion_r1321815229
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/clients/Sidecar.java:
##########
@@ -129,6 +135,9 @@ public static SidecarClient from(SidecarInstancesProvider
sidecarInstancesProvid
.ssl(conf.hasKeystoreAndKeystorePassword())
.build();
+ StartupValidator.instance().register(new
KeystoreValidation(secretsProvider));
+ StartupValidator.instance().register(new
TruststoreValidation(secretsProvider));
+
SidecarConfig sidecarConfig = new SidecarConfig.Builder()
Review Comment:
Unrelated to this PR, but there was a refactoring of configuration, so this
class (SidecarConfig) has been refactored to SidecarClientConfig. You'll need
to do something like this to make it build:
```
SidecarClientConfig sidecarConfig = SidecarClientConfigImpl.builder()
```
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkWriterContext.java:
##########
@@ -21,6 +21,8 @@
import java.io.Serializable;
+import org.apache.cassandra.spark.validation.StartupValidator;
Review Comment:
do we need this import?
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/clients/Sidecar.java:
##########
@@ -129,6 +135,9 @@ public static SidecarClient from(SidecarInstancesProvider
sidecarInstancesProvid
.ssl(conf.hasKeystoreAndKeystorePassword())
.build();
+ StartupValidator.instance().register(new
KeystoreValidation(secretsProvider));
Review Comment:
secretsProvider is only available in the SBR context. For SBW, we'll need to
check whether keystore/password and/or truststore/password are provided, and
validate
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/CassandraValidation.java:
##########
@@ -0,0 +1,24 @@
+package org.apache.cassandra.spark.validation;
Review Comment:
license header is missing
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/SidecarValidation.java:
##########
@@ -0,0 +1,24 @@
+package org.apache.cassandra.spark.validation;
Review Comment:
license header is missing
##########
cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/validation/TestValidation.java:
##########
@@ -0,0 +1,30 @@
+package org.apache.cassandra.spark.validation;
Review Comment:
license header is missing
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/TruststoreValidation.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.cassandra.spark.validation;
+
+import org.apache.cassandra.secrets.SecretsProvider;
+
+public class TruststoreValidation implements StartupValidation
+{
+ private final SecretsProvider secrets;
+
+ public TruststoreValidation(SecretsProvider secrets)
+ {
+ this.secrets = secrets;
+ }
+
+ @Override
+ public void validate()
+ {
+ if (!secrets.hasTrustStoreSecrets())
Review Comment:
hmm, I'm not sure if this is sufficient to validate truststore
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/TruststoreValidation.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.cassandra.spark.validation;
Review Comment:
license header is missing
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/CassandraValidation.java:
##########
@@ -0,0 +1,24 @@
+package org.apache.cassandra.spark.validation;
+
+import org.apache.cassandra.sidecar.client.SidecarClient;
+import org.apache.cassandra.sidecar.common.data.HealthResponse;
+
+public class CassandraValidation implements StartupValidation
+{
+ private final SidecarClient sidecar;
+
+ public CassandraValidation(SidecarClient sidecar)
+ {
+ this.sidecar = sidecar;
+ }
+
+ @Override
+ public void validate()
+ {
+ HealthResponse health = sidecar.cassandraHealth().get();
Review Comment:
wouldn't Cassandra Health endpoint cover both sidecar health and Cassandra's
health? Do we need to perform both calls?
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/KeystoreValidation.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.cassandra.spark.validation;
Review Comment:
license header is missing
##########
profiles/scala-2.11-spark-2-jdk-1.8.gradle:
##########
@@ -26,5 +26,5 @@ ext {
sparkVersion="2.4.8"
sidecarClientGroup="org.apache.cassandra.sidecar"
sidecarClientName="vertx-client-all"
- sidecarVersion="1.0.0-jdk8-local"
+ sidecarVersion="1.0.0.44-jdk8-local"
Review Comment:
this seems incorrect. we should use 1.0.0-jdk8-local for java8 and
1.0.0-local for jdk11
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/StartupValidator.java:
##########
@@ -0,0 +1,36 @@
+package org.apache.cassandra.spark.validation;
Review Comment:
license header is missing
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/validation/SidecarValidation.java:
##########
@@ -0,0 +1,24 @@
+package org.apache.cassandra.spark.validation;
+
+import org.apache.cassandra.sidecar.client.SidecarClient;
+import org.apache.cassandra.sidecar.common.data.HealthResponse;
+
+public class SidecarValidation implements StartupValidation
+{
+ private final SidecarClient sidecar;
+
+ public SidecarValidation(SidecarClient sidecar)
+ {
+ this.sidecar = sidecar;
+ }
+
+ @Override
+ public void validate()
+ {
+ HealthResponse health = sidecar.sidecarHealth().get();
Review Comment:
should we get with a timeout? you don't want to block indefinitely
--
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: commits-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
