yifan-c commented on code in PR #15:
URL:
https://github.com/apache/cassandra-analytics/pull/15#discussion_r1330930867
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/BulkSparkConf.java:
##########
@@ -203,7 +203,7 @@ else if (!batchSizeIsZero && sstableBatchSize !=
DEFAULT_BATCH_SIZE_IN_ROWS)
/*
* This method Will throw if the SSL configuration is incorrect (PATH
provided w/o password, for example)
*/
- protected void validateSslConfiguration()
+ public void validateSslConfiguration()
Review Comment:
Can you enrich the javadoc? It is now a public method. I think it should
cover the possible exceptions and all the scenarios when it could throw.
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/CassandraContext.java:
##########
@@ -96,4 +100,14 @@ protected BulkSparkConf conf()
{
return conf;
}
+
+ // Startup Validation
+
+ @Override
+ public void startupValidate()
+ {
+ StartupValidator.instance().register(new
SidecarValidation(sidecarClient));
+ StartupValidator.instance().register(new
CassandraValidation(sidecarClient));
+ StartupValidator.instance().perform();
Review Comment:
What is the benefit of registering and performing validation immediately?
It would be simpler to just run the 2 validations inline.
##########
cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/validation/KeyStoreValidationTests.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.spark.validation;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import org.apache.cassandra.secrets.SecretsProvider;
+import org.apache.cassandra.secrets.TestSecretsProvider;
+
+/**
+ * Unit tests that cover startup validation of a KeyStore
+ */
+public class KeyStoreValidationTests
+{
+ @Test()
Review Comment:
redundant parentheses. please remove all of them, i.e. `@Test()` --> `@Test`
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/data/CassandraDataLayer.java:
##########
@@ -639,6 +644,16 @@ public CassandraRing
createCassandraRingFromRing(Partitioner partitioner,
return new CassandraRing(partitioner, keyspace, replicationFactor,
instances);
}
+ // Startup Validation
+
+ @Override
+ public void startupValidate()
+ {
+ StartupValidator.instance().register(new SidecarValidation(sidecar));
+ StartupValidator.instance().register(new CassandraValidation(sidecar));
+ StartupValidator.instance().perform();
+ }
Review Comment:
Same comment here. It feel to me that the interface validate-able is
tangling/clashing with the validator/validation classes.
##########
cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/validation/KeyStoreValidationTests.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.spark.validation;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import org.apache.cassandra.secrets.SecretsProvider;
+import org.apache.cassandra.secrets.TestSecretsProvider;
+
+/**
+ * Unit tests that cover startup validation of a KeyStore
+ */
+public class KeyStoreValidationTests
+{
+ @Test()
+ public void testNullSecrets()
+ {
+ KeyStoreValidation validation = new KeyStoreValidation(null);
+
+ Assertions.assertThrows(RuntimeException.class, validation::perform);
Review Comment:
can you assert on the thrown exception's message, and for the other test
cases? There could be other places that throw `RuntimeException`.
You can achieve such assertion with this code example
```java
RuntimeException rte =
Assertions.assertThrows(RuntimeException.class, validation::perform);
Assertions.assertTrue(rte.getMessage().contains("blabla"));
```
--
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]
