frankgh commented on code in PR #14:
URL: 
https://github.com/apache/cassandra-analytics/pull/14#discussion_r1326688112


##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/clients/Sidecar.java:
##########
@@ -129,16 +130,15 @@ public static SidecarClient from(SidecarInstancesProvider 
sidecarInstancesProvid
                                             
.ssl(conf.hasKeystoreAndKeystorePassword())
                                             .build();
 
-        SidecarConfig sidecarConfig = new SidecarConfig.Builder()
-                                      .maxRetries(5)
-                                      .retryDelayMillis(200)
-                                      .maxRetryDelayMillis(500)
-                                      .build();
-
-        return buildClient(sidecarConfig, vertx, httpClientConfig, 
sidecarInstancesProvider);
+        SidecarClientConfig sidecarConfig = SidecarClientConfigImpl.builder()
+                                                                       
.maxRetries(5)
+                                                                       
.retryDelayMillis(200)
+                                                                       
.maxRetryDelayMillis(500)
+                                                                       
.build();
+        return buildClient((SidecarClientConfigImpl) sidecarConfig, vertx, 
httpClientConfig, sidecarInstancesProvider);
     }
 
-    public static SidecarClient buildClient(SidecarConfig sidecarConfig,
+    public static SidecarClient buildClient(SidecarClientConfigImpl 
sidecarConfig,

Review Comment:
   Can we use the interface here instead?
   ```suggestion
       public static SidecarClient buildClient(SidecarClientConfig 
sidecarConfig,
   ```



##########
build.gradle:
##########
@@ -150,8 +166,15 @@ subprojects {
 
   repositories {
     mavenCentral()
+      mavenLocal {

Review Comment:
   indentation.



##########
scripts/build-dependencies.sh:
##########
@@ -0,0 +1,23 @@
+#!/bin/bash
+#
+# 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.
+#
+
+SCRIPT_DIR=$( dirname -- "$( readlink -f -- "$0"; )"; )
+
+${SCRIPT_DIR}/build-dtest-jars.sh
+${SCRIPT_DIR}/build-sidecar.sh

Review Comment:
   can we add a flag to conditionally build dtest-jars and/or sidecar?



##########
gradle.properties:
##########
@@ -29,5 +29,6 @@ jnaVersion=5.9.0
 scala=2.12
 spark=3
 vertxVersion=4.2.1
+sidecarVersion=1.0.0-analytics

Review Comment:
   this is already part of profiles, we shouldn't have it here



##########
build.gradle:
##########
@@ -68,9 +77,10 @@ tasks.idea.dependsOn(tasks.copyInspections)
 
 tasks.register('buildIgnoreRatList', Exec) {
   description 'Builds a list of ignored files for the rat task from the 
unversioned git files'
-  commandLine 'bash', '-c', 'git clean --force -d --dry-run -x | cut -c 14-'
+  commandLine 'bash', '-c', 'git clean --force -d --dry-run -x | grep -v 
"Would skip" | cut -c 14- && ' +
+          'git clean --force -d --dry-run -x | grep "Would skip" | cut -c 
23-\n'
   doFirst {
-    standardOutput new FileOutputStream("${buildDir}/.rat-excludes.txt")
+    standardOutput new FileOutputStream(".rat-excludes.txt")

Review Comment:
   why not keep it in the build dir? It is useful to debug, because these 
resources are copied in CI to resources



##########
cassandra-bridge/src/main/java/org/apache/cassandra/bridge/CassandraVersion.java:
##########
@@ -29,7 +29,9 @@ public enum CassandraVersion
 {
     THREEZERO(30, "3.0", "three-zero"),
     FOURZERO(40, "4.0", "four-zero"),
-    FOURONE(41, "4.1", "four-zero");
+    FOURONE(41, "4.1", "four-zero"),
+    FIVEZERO(50, "5.0", "four-zero"),

Review Comment:
   hmm, this feels like it's outside of the scope of the project. I also feel 
that the four-zero implementation won't be sufficient to support 5.0, so I 
would prefer to have a proper five-zero implementation.



-- 
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

Reply via email to