frankgh commented on code in PR #299:
URL: https://github.com/apache/cassandra-sidecar/pull/299#discussion_r2629128563
##########
scripts/build-dtest-jars.sh:
##########
@@ -19,11 +19,10 @@
set -xe
CANDIDATE_BRANCHES=(
- "cassandra-4.0:64b8d6b9add607b80752cd1a8fbce51839af9ec4"
- "cassandra-4.1:044727aabafeab2f6fef74c52d349d55c8732ef5"
- "cassandra-5.0:a0d58a9ce8814d096c1bd8a0440e8e28d8ea15a9"
- # note the trunk hash cannot be advanced beyond
ae0842372ff6dd1437d026f82968a3749f555ff4 (TCM), which breaks integration test
- "trunk:2a5e1b77c9f8a205dbec1afdea3f4ed1eaf6a4eb"
+ "cassandra-4.0:aa0e2f1631ae343e35334e5419b193a9a1cfa0a6"
Review Comment:
can we move to the latest? `4c33f1f2d7672ed9eef08908eb86a262d9fdc35b`
##########
server-common/build.gradle:
##########
@@ -26,11 +26,16 @@ plugins {
id 'java-test-fixtures'
}
apply from: "$rootDir/gradle/common/publishing.gradle"
+apply from: "${project.rootDir}/gradle/common/java11Options.gradle"
sourceCompatibility = JavaVersion.VERSION_11
test {
useJUnitPlatform()
+ if (JavaVersion.current().isJava11Compatible()) {
Review Comment:
Is this always true?
##########
adapters/adapters-base/src/main/java/org/apache/cassandra/sidecar/adapters/base/CassandraStorageOperations.java:
##########
@@ -132,10 +133,19 @@ protected void takeSnapshotInternal(@NotNull String tag,
}
catch (IOException e)
{
+ // post-5.0, IOExceptions are thrown when previously something
else like
+ // an IllegalArgumentException was thrown. First, try to unwrap
the IOException and throw
+ // the original cause, which we will process correctly in
CreateSnapshotHandler
+ // if the exception was an IllegalArgumentException
+ IllegalArgumentException iex = ThrowableUtils.getCause(e,
IllegalArgumentException.class);
+ if (iex != null) {
Review Comment:
the formatting seems off in some places
```suggestion
if (iex != null)
{
```
##########
server/src/test/integration/org/apache/cassandra/sidecar/testing/IntegrationTestModule.java:
##########
@@ -119,7 +119,7 @@ public SidecarConfiguration
configuration(CoordinationConfiguration clusterLease
PeriodicTaskConfiguration healthCheckConfiguration
= new PeriodicTaskConfigurationImpl(true,
MillisecondBoundConfiguration.parse("50ms"),
-
MillisecondBoundConfiguration.parse("500ms"));
+
MillisecondBoundConfiguration.parse("5000ms"));
Review Comment:
any reason we reduce the periodic health check frequency?
##########
scripts/build-dtest-jars.sh:
##########
@@ -19,11 +19,10 @@
set -xe
CANDIDATE_BRANCHES=(
- "cassandra-4.0:64b8d6b9add607b80752cd1a8fbce51839af9ec4"
- "cassandra-4.1:044727aabafeab2f6fef74c52d349d55c8732ef5"
- "cassandra-5.0:a0d58a9ce8814d096c1bd8a0440e8e28d8ea15a9"
- # note the trunk hash cannot be advanced beyond
ae0842372ff6dd1437d026f82968a3749f555ff4 (TCM), which breaks integration test
- "trunk:2a5e1b77c9f8a205dbec1afdea3f4ed1eaf6a4eb"
+ "cassandra-4.0:aa0e2f1631ae343e35334e5419b193a9a1cfa0a6"
+ "cassandra-4.1:c988b609b0239e37f37e3b764728d960220cc3e8"
+ "cassandra-5.0:b4dcef78419c29584937b44aa484cf0c13cf37e0"
+ "trunk:9142d0c8519944e02b3d449b21c3b42ab80caeb6"
Review Comment:
```suggestion
"trunk:d2c48faf71936084216c2dd7b164d6eae5cabbd3"
```
##########
gradle/common/java11Options.gradle:
##########
@@ -23,12 +23,22 @@ project.ext.JDK11_OPTIONS =
['-Djdk.attach.allowAttachSelf=true',
'--add-exports',
'java.management.rmi/com.sun.jmx.remote.internal.rmi=ALL-UNNAMED',
'--add-exports',
'java.rmi/sun.rmi.registry=ALL-UNNAMED',
'--add-exports',
'java.rmi/sun.rmi.server=ALL-UNNAMED',
+ '--add-exports',
'java.rmi/sun.rmi.transport=ALL-UNNAMED',
Review Comment:
it'd be nice to document which flags are needed for JDK 11 and which ones
are needed for JDK 17
##########
scripts/build-dtest-jars.sh:
##########
@@ -19,11 +19,10 @@
set -xe
CANDIDATE_BRANCHES=(
- "cassandra-4.0:64b8d6b9add607b80752cd1a8fbce51839af9ec4"
- "cassandra-4.1:044727aabafeab2f6fef74c52d349d55c8732ef5"
- "cassandra-5.0:a0d58a9ce8814d096c1bd8a0440e8e28d8ea15a9"
- # note the trunk hash cannot be advanced beyond
ae0842372ff6dd1437d026f82968a3749f555ff4 (TCM), which breaks integration test
- "trunk:2a5e1b77c9f8a205dbec1afdea3f4ed1eaf6a4eb"
+ "cassandra-4.0:aa0e2f1631ae343e35334e5419b193a9a1cfa0a6"
+ "cassandra-4.1:c988b609b0239e37f37e3b764728d960220cc3e8"
Review Comment:
```suggestion
"cassandra-4.1:efa0ead445b4f778abebf17835bf6947ffcdff0f"
```
##########
server/src/test/integration/org/apache/cassandra/sidecar/routes/tokenrange/ReplacementTest.java:
##########
@@ -35,6 +35,7 @@
import io.vertx.junit5.VertxExtension;
import io.vertx.junit5.VertxTestContext;
+import net.bytebuddy.implementation.bind.annotation.RuntimeType;
Review Comment:
can you remove unused imports?
##########
scripts/build-dtest-jars.sh:
##########
@@ -19,11 +19,10 @@
set -xe
CANDIDATE_BRANCHES=(
- "cassandra-4.0:64b8d6b9add607b80752cd1a8fbce51839af9ec4"
- "cassandra-4.1:044727aabafeab2f6fef74c52d349d55c8732ef5"
- "cassandra-5.0:a0d58a9ce8814d096c1bd8a0440e8e28d8ea15a9"
- # note the trunk hash cannot be advanced beyond
ae0842372ff6dd1437d026f82968a3749f555ff4 (TCM), which breaks integration test
- "trunk:2a5e1b77c9f8a205dbec1afdea3f4ed1eaf6a4eb"
+ "cassandra-4.0:aa0e2f1631ae343e35334e5419b193a9a1cfa0a6"
+ "cassandra-4.1:c988b609b0239e37f37e3b764728d960220cc3e8"
+ "cassandra-5.0:b4dcef78419c29584937b44aa484cf0c13cf37e0"
Review Comment:
```suggestion
"cassandra-5.0:f894b8440defd0693ef9e84dbef1f790959af7fe"
```
##########
server/src/test/integration/org/apache/cassandra/sidecar/routes/tokenrange/BaseTokenRangeIntegrationTest.java:
##########
@@ -305,4 +332,20 @@ private void
validateRanges(List<TokenRangeReplicasResponse.ReplicaInfo> replica
.map(TokenRangeReplicasResponse.ReplicaInfo::end)
.anyMatch(s ->
s.equals("9223372036854775807"))).isTrue();
}
+
+ protected @NotNull Set<String> getDcReplication(CassandraIntegrationTest
annotation)
+ {
+ Set<String> dcReplication;
+ if (annotation.numDcs() > 1)
+ {
+ createTestKeyspace(ImmutableMap.of("replication_factor",
DEFAULT_RF));
+ dcReplication = Sets.newHashSet(Arrays.asList("datacenter1",
"datacenter2"));
Review Comment:
NIT: use JDK primitive types
```suggestion
createTestKeyspace(Map.of("replication_factor", DEFAULT_RF));
dcReplication = Set.of("datacenter1", "datacenter2");
```
##########
server/src/test/java/org/apache/cassandra/sidecar/handlers/cdc/StreamCdcSegmentHandlerTest.java:
##########
@@ -268,7 +283,7 @@ private void spinAssertCdcRawTempEmpty()
File cdcTempDir = new File(CDC_RAW_TEMP_DIR);
assertThat(cdcTempDir.exists()).isTrue();
- int attempts = 10;
+ int attempts = 20;
Review Comment:
use loopAssert instead?
##########
server/src/test/integration/org/apache/cassandra/sidecar/routes/tokenrange/MovingMultiDCTest.java:
##########
@@ -186,15 +187,48 @@ public static void install(ClassLoader cl, Integer
nodeNumber)
if (nodeNumber == MULTIDC_MOVING_NODE_IDX)
{
TypePool typePool = TypePool.Default.of(cl);
- TypeDescription description =
typePool.describe("org.apache.cassandra.service.RangeRelocator")
- .resolve();
- new ByteBuddy().rebase(description,
ClassFileLocator.ForClassLoader.of(cl))
- .method(named("stream"))
-
.intercept(MethodDelegation.to(BBHelperMovingNodeMultiDC.class))
- // Defer class loading until all dependencies
are loaded
- .make(TypeResolutionStrategy.Lazy.INSTANCE,
typePool)
- .load(cl,
ClassLoadingStrategy.Default.INJECTION);
+ if (installPreTcm(cl, typePool))
+ {
+ return;
+ }
+
+ if (installTcm(cl, typePool)) {
Review Comment:
formatting?
##########
server/src/test/integration/org/apache/cassandra/sidecar/routes/tokenrange/JoiningBaseTest.java:
##########
@@ -46,6 +46,7 @@
import org.apache.cassandra.testing.CassandraIntegrationTest;
import org.apache.cassandra.testing.ConfigurableCassandraTestContext;
import org.apache.cassandra.testing.IClusterExtension;
+import org.jetbrains.annotations.NotNull;
Review Comment:
please remove unused imports
##########
server/src/test/integration/org/apache/cassandra/sidecar/testing/TestTokenSupplier.java:
##########
@@ -20,17 +20,21 @@
import java.math.BigInteger;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.Collections;
import java.util.List;
import org.apache.cassandra.distributed.api.TokenSupplier;
+import org.jetbrains.annotations.NotNull;
/**
* Static factory holder that provides a token supplier
*/
-public class TestTokenSupplier
+public class TestTokenSupplier implements TokenSupplier
{
+ private List<String>[] tokens;
Review Comment:
NIT: make it final
```suggestion
private final List<String>[] tokens;
```
##########
server/src/test/integration/org/apache/cassandra/sidecar/routes/tokenrange/BaseTokenRangeIntegrationTest.java:
##########
@@ -305,4 +332,20 @@ private void
validateRanges(List<TokenRangeReplicasResponse.ReplicaInfo> replica
.map(TokenRangeReplicasResponse.ReplicaInfo::end)
.anyMatch(s ->
s.equals("9223372036854775807"))).isTrue();
}
+
+ protected @NotNull Set<String> getDcReplication(CassandraIntegrationTest
annotation)
+ {
+ Set<String> dcReplication;
+ if (annotation.numDcs() > 1)
+ {
+ createTestKeyspace(ImmutableMap.of("replication_factor",
DEFAULT_RF));
+ dcReplication = Sets.newHashSet(Arrays.asList("datacenter1",
"datacenter2"));
+ }
+ else
+ {
+ createTestKeyspace(ImmutableMap.of("datacenter1", DEFAULT_RF));
+ dcReplication = Collections.singleton("datacenter1");
Review Comment:
NIT: use JDK primitive types
```suggestion
createTestKeyspace(Map.of("datacenter1", DEFAULT_RF));
dcReplication = Set.of("datacenter1");
```
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/utils/StringUtils.java:
##########
@@ -46,4 +48,12 @@ public static boolean isNotEmpty(@Nullable String string)
{
return !isNullOrEmpty(string);
}
+
+ public static boolean contains(@NotNull String string, @NotNull String
target) {
Review Comment:
Can we add a javadoc for this method?
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/utils/StringUtils.java:
##########
@@ -46,4 +48,12 @@ public static boolean isNotEmpty(@Nullable String string)
{
return !isNullOrEmpty(string);
}
+
+ public static boolean contains(@NotNull String string, @NotNull String
target) {
Review Comment:
`guava` could cause some issues when pulling it into spark for example.
`client-common` needs to be lean and have the minimum number of dependencies.
##########
server/src/test/integration/org/apache/cassandra/sidecar/testing/bytebuddy/BBHelperLeavingNode.java:
##########
@@ -47,20 +47,55 @@ public static void install(ClassLoader cl, int nodeNum, int
leavingNodeNum)
{
if (nodeNum == leavingNodeNum)
{
- TypePool typePool = TypePool.Default.of(cl);
- TypeDescription description =
typePool.describe("org.apache.cassandra.service.StorageService")
- .resolve();
- new ByteBuddy().rebase(description,
ClassFileLocator.ForClassLoader.of(cl))
- .method(named("unbootstrap"))
-
.intercept(MethodDelegation.to(BBHelperLeavingNode.class))
- // Defer class loading until all dependencies are
loaded
- .make(TypeResolutionStrategy.Lazy.INSTANCE,
typePool)
- .load(cl, ClassLoadingStrategy.Default.INJECTION);
+ intercept(cl, BBHelperLeavingNode.class);
}
}
+ public static void intercept(ClassLoader cl, Class<?> delegateClass)
+ {
+ TypePool typePool = TypePool.Default.of(cl);
+ if (installTCM(cl, typePool, delegateClass) || installPreTCM(cl,
typePool, delegateClass)) {
Review Comment:
formatting
--
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]
