frankgh commented on code in PR #62:
URL:
https://github.com/apache/cassandra-analytics/pull/62#discussion_r1672572965
##########
cassandra-four-zero-bridge/build.gradle:
##########
@@ -29,13 +29,15 @@ configurations {
}
dependencies {
+ compileOnly project(":cassandra-analytics-common")
compileOnly(project(':cassandra-bridge'))
compileOnly(group: "${sparkGroupId}", name:
"spark-core_${scalaMajorVersion}", version:
"${project.rootProject.sparkVersion}")
compileOnly(group: "${sparkGroupId}", name:
"spark-sql_${scalaMajorVersion}", version:
"${project.rootProject.sparkVersion}")
compileOnly(project(path: ':cassandra-four-zero', configuration: 'shadow'))
+ testImplementation project(":cassandra-analytics-common")
Review Comment:
also not needed if we make the change in bridge
##########
gradle.properties:
##########
@@ -28,6 +28,10 @@ mockitoVersion=3.12.4
jnaVersion=5.9.0
scala=2.12
spark=3
+kryoVersion=4.0.2
+commonsLangVersion=2.6
Review Comment:
can we remove this?
##########
gradle.properties:
##########
@@ -28,6 +28,10 @@ mockitoVersion=3.12.4
jnaVersion=5.9.0
scala=2.12
spark=3
+kryoVersion=4.0.2
+commonsLangVersion=2.6
+slf4jApiVersion=1.7.30
Review Comment:
nice! Can we update all places that use slf4j to use this variable ?
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/reader/EmptyStreamScanner.java:
##########
@@ -24,13 +24,13 @@ public class EmptyStreamScanner implements
StreamScanner<Rid>
public static final EmptyStreamScanner INSTANCE = new EmptyStreamScanner();
@Override
- public Rid rid()
+ public Rid data()
{
return null;
}
@Override
- public boolean hasNext()
+ public boolean next()
Review Comment:
I think the method name `hasNext` makes more sense than `next` . Next would
make sense if it returned the actual value. Any reason for changing it? Can we
restore it?
##########
cassandra-bridge/build.gradle:
##########
@@ -39,6 +39,7 @@ publishing {
}
dependencies {
+ implementation(project(':cassandra-analytics-common'))
Review Comment:
We should just declare this as an API implementation so we don't need to add
both the bridge and common as dependencies in other subprojects (i.e four-zero)
```suggestion
api(project(':cassandra-analytics-common'))
```
##########
cassandra-four-zero-bridge/build.gradle:
##########
@@ -29,13 +29,15 @@ configurations {
}
dependencies {
+ compileOnly project(":cassandra-analytics-common")
Review Comment:
not needed if we declare it as api in the bridge. It will be brought as a
transitive dep that can be used from the project
##########
cassandra-analytics-common/build.gradle:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id('java-library')
+ id('maven-publish')
+}
+
+java {
+ withJavadocJar()
+ withSourcesJar()
+}
+
+publishing {
+ publications {
+ maven(MavenPublication) {
+ from components.java
+ groupId project.group
+ artifactId "${archivesBaseName}"
+ version System.getenv("CODE_VERSION") ?: "${version}"
+ }
+ }
+}
+
+dependencies {
+ implementation "org.slf4j:slf4j-api:${slf4jApiVersion}"
+ implementation "com.esotericsoftware:kryo-shaded:${kryoVersion}"
+ implementation "commons-lang:commons-lang:${commonsLangVersion}"
Review Comment:
we've avoided adding commons-lang dependencies because they tend to clash
with spark provided commons-lang. I see it's only being used for hash builder
and equality checking. This is easily done with native JDK APIs. Can you avoid
adding this dependency?
##########
cassandra-analytics-common/build.gradle:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id('java-library')
+ id('maven-publish')
+}
+
+java {
+ withJavadocJar()
+ withSourcesJar()
+}
+
+publishing {
+ publications {
+ maven(MavenPublication) {
+ from components.java
+ groupId project.group
+ artifactId "${archivesBaseName}"
+ version System.getenv("CODE_VERSION") ?: "${version}"
+ }
+ }
+}
+
+dependencies {
+ implementation "org.slf4j:slf4j-api:${slf4jApiVersion}"
+ implementation "com.esotericsoftware:kryo-shaded:${kryoVersion}"
Review Comment:
hmm. this dependency used to be a transitive dependency through spark-core.
Can we instead have it as a compileOnly dependency here instead?
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/data/LocalDataLayer.java:
##########
@@ -174,7 +174,7 @@ public static LocalDataLayer from(Map<String, String>
options)
.filter(StringUtils::isNotEmpty)
.collect(Collectors.toSet()),
SchemaFeatureSet.initializeFromOptions(options),
- getBoolean(options, lowerCaseKey("useSSTableInputStream"),
false),
+ getBoolean(options, lowerCaseKey("useBufferingInputStream"),
getBoolean(options, lowerCaseKey("useSSTableInputStream"), false)),
Review Comment:
this is a behavior change. We should support the old property also for
backwards compatibility
##########
cassandra-analytics-common/build.gradle:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id('java-library')
+ id('maven-publish')
+}
+
+java {
Review Comment:
this gradle file is missing the test configuration section. You won't be
able to run unit tests from some IDE environments for example without it. Also,
test reporting will be missing for tests in this subproject. Please refer to
other subprojects for reference
##########
cassandra-analytics-common/build.gradle:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id('java-library')
+ id('maven-publish')
+}
+
+java {
+ withJavadocJar()
+ withSourcesJar()
+}
+
+publishing {
+ publications {
+ maven(MavenPublication) {
+ from components.java
+ groupId project.group
+ artifactId "${archivesBaseName}"
+ version System.getenv("CODE_VERSION") ?: "${version}"
+ }
+ }
+}
+
+dependencies {
+ implementation "org.slf4j:slf4j-api:${slf4jApiVersion}"
+ implementation "com.esotericsoftware:kryo-shaded:${kryoVersion}"
+ implementation "commons-lang:commons-lang:${commonsLangVersion}"
+ implementation "com.google.guava:guava:${guavaVersion}"
Review Comment:
I think for guava we also need to bring it as compile only as it will
resolve some version in the spark runtime environment
##########
gradle.properties:
##########
@@ -28,6 +28,10 @@ mockitoVersion=3.12.4
jnaVersion=5.9.0
scala=2.12
spark=3
+kryoVersion=4.0.2
+commonsLangVersion=2.6
+slf4jApiVersion=1.7.30
+guavaVersion=16.0.1
Review Comment:
any reason we choose this version of guava? it seems old, and we have
different versions in this project?
##########
cassandra-analytics-integration-tests/build.gradle:
##########
@@ -63,6 +63,7 @@ configurations {
}
dependencies {
+ testImplementation(project(':cassandra-analytics-common'))
Review Comment:
this dependency should come from core. No need to declare it here.
##########
cassandra-bridge/src/main/java/org/apache/cassandra/spark/utils/FastThreadLocalUtf8Decoder.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.utils;
+
+import java.nio.ByteBuffer;
+import java.nio.charset.CharacterCodingException;
+import java.nio.charset.CharsetDecoder;
+import java.nio.charset.StandardCharsets;
+
+import io.netty.util.concurrent.FastThreadLocal;
+
+public final class FastThreadLocalUtf8Decoder
+{
+ private FastThreadLocalUtf8Decoder()
+ {
+ super();
+ throw new IllegalStateException(getClass() + " is static utility class
and shall not be instantiated");
+ }
+
+ private static final FastThreadLocal<CharsetDecoder> UTF8_DECODER = new
FastThreadLocal<CharsetDecoder>()
+ {
+ @Override
+ protected CharsetDecoder initialValue()
+ {
+ return StandardCharsets.UTF_8.newDecoder();
+ }
+ };
+
+ public static String string(ByteBuffer buffer) throws
CharacterCodingException
+ {
+ return ByteBufferUtils.string(buffer,
FastThreadLocalUtf8Decoder.UTF8_DECODER::get);
+ }
+
+ public static String stringThrowRuntime(ByteBuffer buffer)
+ {
+ try
+ {
+ return string(buffer);
+ }
+ catch (final CharacterCodingException exception)
Review Comment:
NIT : avoid using final keyword here
```suggestion
catch (CharacterCodingException exception)
```
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/utils/ByteBufferUtils.java:
##########
@@ -30,20 +30,13 @@
import java.nio.charset.CharsetDecoder;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
-
-import io.netty.util.concurrent.FastThreadLocal;
+import java.util.function.Supplier;
public final class ByteBufferUtils
{
+ public static final ThreadLocal<CharsetDecoder> UTF8_DECODER_PROVIDER =
ThreadLocal.withInitial(StandardCharsets.UTF_8::newDecoder);
Review Comment:
Any reason we need to make it public? I don't see it being used anywhere
else.
##########
cassandra-analytics-common/build.gradle:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id('java-library')
+ id('maven-publish')
+}
+
+java {
+ withJavadocJar()
+ withSourcesJar()
+}
+
+publishing {
+ publications {
+ maven(MavenPublication) {
+ from components.java
+ groupId project.group
+ artifactId "${archivesBaseName}"
+ version System.getenv("CODE_VERSION") ?: "${version}"
+ }
+ }
+}
+
+dependencies {
+ implementation "org.slf4j:slf4j-api:${slf4jApiVersion}"
+ implementation "com.esotericsoftware:kryo-shaded:${kryoVersion}"
+ implementation "commons-lang:commons-lang:${commonsLangVersion}"
+ implementation "com.google.guava:guava:${guavaVersion}"
Review Comment:
ah yes, it's coming transitively provided by the spark runtime environment.
I'd move it to compile only to avoid problems
--
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
