-
Notifications
You must be signed in to change notification settings - Fork 52
CASSSIDECAR-277 Support testing of post-TCM Cassandra #299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,6 +40,7 @@ | |||||||
| import org.apache.cassandra.sidecar.common.server.dns.DnsResolver; | ||||||||
| import org.apache.cassandra.sidecar.common.server.exceptions.NodeBootstrappingException; | ||||||||
| import org.apache.cassandra.sidecar.common.server.exceptions.SnapshotAlreadyExistsException; | ||||||||
| import org.apache.cassandra.sidecar.common.server.utils.ThrowableUtils; | ||||||||
| import org.jetbrains.annotations.NotNull; | ||||||||
| import org.jetbrains.annotations.Nullable; | ||||||||
|
|
||||||||
|
|
@@ -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) { | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the formatting seems off in some places
Suggested change
|
||||||||
| throw iex; | ||||||||
| } | ||||||||
| String errorMessage = e.getMessage(); | ||||||||
| if (errorMessage != null) | ||||||||
| { | ||||||||
| if (errorMessage.contains("Snapshot " + tag + " already exists")) | ||||||||
| if (errorMessage.contains("Snapshot " + tag + " already exists") || | ||||||||
| errorMessage.contains("Snapshot " + tag + " for " + keyspace + "." + table + " already exists")) | ||||||||
| { | ||||||||
| throw new SnapshotAlreadyExistsException(e); | ||||||||
| } | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ | |
|
|
||
| package org.apache.cassandra.sidecar.common.utils; | ||
|
|
||
| import java.util.Objects; | ||
| import org.jetbrains.annotations.NotNull; | ||
| import org.jetbrains.annotations.Nullable; | ||
|
|
||
| /** | ||
|
|
@@ -46,4 +48,12 @@ public static boolean isNotEmpty(@Nullable String string) | |
| { | ||
| return !isNullOrEmpty(string); | ||
| } | ||
|
|
||
| public static boolean contains(@NotNull String string, @NotNull String target) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not see a reason why we should have the StringUtils class. For all other methods in this class can be removed and Guava StringUtils could be used, for this method can use If you think that the strings could be null, then there is a mismatch in the annotation for parameters
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Short answer to "why we should have the StringUtils class" is that we do not want to add the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a javadoc for this method? |
||
| if (isNullOrEmpty(string) || isNullOrEmpty(target)) | ||
| { | ||
| return false; | ||
| } | ||
| return string.contains(target); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it'd be nice to document which flags are needed for JDK 11 and which ones are needed for JDK 17 |
||
| '--add-exports', 'java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED', | ||
| '--add-exports', 'java.sql/java.sql=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.io=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/sun.nio.ch=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.lang=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.lang.module=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.lang.reflect=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.util=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.util.concurrent=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/java.util.concurrent.atomic=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/jdk.internal.loader=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/jdk.internal.ref=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/jdk.internal.reflect=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/jdk.internal.math=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/jdk.internal.module=ALL-UNNAMED', | ||
| '--add-opens', 'java.base/jdk.internal.util.jar=ALL-UNNAMED', | ||
| '--add-opens', 'java.rmi/sun.rmi.transport.tcp=ALL-UNNAMED', | ||
| '--add-opens', 'jdk.management/com.sun.management.internal=ALL-UNNAMED'] | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we move to the latest? |
||||||
| "cassandra-4.1:c988b609b0239e37f37e3b764728d960220cc3e8" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| "cassandra-5.0:b4dcef78419c29584937b44aa484cf0c13cf37e0" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| "trunk:9142d0c8519944e02b3d449b21c3b42ab80caeb6" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ) | ||||||
| BRANCHES=( ${BRANCHES:-cassandra-4.0 cassandra-4.1 cassandra-5.0 trunk} ) | ||||||
| echo ${BRANCHES[*]} | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this always true? |
||
| jvmArgs(project.ext.JDK11_OPTIONS) | ||
| println("JVM arguments for $project.name are $allJvmArgs") | ||
| } | ||
| maxParallelForks = Runtime.runtime.availableProcessors().intdiv(2) ?: 1 | ||
| reports { | ||
| junitXml.setRequired(true) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ | |
| /** | ||
| * Handler for enabling or disabling live migration APIs based on instance role and migration status. | ||
| * <p> | ||
| * <h3>Handler Methods:</h3> | ||
| * <h2>Handler Methods:</h2> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we not change those docs? They are not relevant.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are relevant in that different JDKs fail javadoc generation if they are the "wrong" level, which is why they were changed here. I removed some of them from another class for the same reason, so maybe we just remove them all together to avoid the issue? |
||
| * <ul> | ||
| * <li>{@link #isSource(RoutingContext)} - Allows access only if instance is a migration source</li> | ||
| * <li>{@link #isDestination(RoutingContext)} - Allows access only if instance is a migration destination</li> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍