You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "JeetKunDoug (via GitHub)" <gi...@apache.org> on 2023/06/15 00:44:30 UTC
[GitHub] [cassandra-sidecar] JeetKunDoug opened a new pull request, #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
JeetKunDoug opened a new pull request, #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51
This also required moving some additional classes to `common` so they could be used in the new test-framework project.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1277452661
##########
build.gradle:
##########
@@ -20,6 +20,7 @@
import java.nio.file.Files
import java.nio.file.Paths
+import java.nio.file.Files
Review Comment:
odd, this is imported already in line 21
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1279885360
##########
adapters/base/build.gradle:
##########
@@ -59,7 +60,7 @@ publishing {
maven(MavenPublication) {
from components.java
groupId project.group
- artifactId "${archivesBaseName}"
+ artifactId "adapters-base"
Review Comment:
`adapters-base` matches the directory structure `adapters/base` but I suppose `base-adapter` rolls off the tongue a bit more easily - @yifan-c any opinions here (I'll leave it as is until we come to consensus but don't have a strong opinion either way)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1279830156
##########
adapters/base/build.gradle:
##########
@@ -59,7 +60,7 @@ publishing {
maven(MavenPublication) {
from components.java
groupId project.group
- artifactId "${archivesBaseName}"
+ artifactId "adapters-base"
Review Comment:
how about `base-adapter`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1278105567
##########
build.gradle:
##########
@@ -20,6 +20,7 @@
import java.nio.file.Files
import java.nio.file.Paths
+import java.nio.file.Files
Review Comment:
Fixed.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1280677963
##########
src/test/integration/org/apache/cassandra/sidecar/testing/CassandraSidecarTestContext.java:
##########
@@ -142,6 +182,8 @@ public String toString()
public void close()
{
instancesConfig.instances().forEach(instance -> instance.delegate().close());
+ // TODO: Do we need to close sessions?
+// sessionProviders.
Review Comment:
Yup - thanks for catching it... was a simple one-liner I just hadn't remember to get back to.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1280880325
##########
common/src/main/java/org/apache/cassandra/sidecar/cluster/InstancesConfigImpl.java:
##########
@@ -85,10 +85,12 @@ public InstanceMetadata instanceFromHost(String host) throws NoSuchElementExcept
}
catch (UnknownHostException e)
{
- throw new NoSuchElementException("Instance with host address "
- + host +
- " not found, and an error occurred when " +
- "attempting to get its IP address.");
+ NoSuchElementException error = new NoSuchElementException("Instance with host address "
+ + host +
+ " not found, and an error occurred when " +
+ "attempting to resolve its IP address.");
+ error.initCause(e);
Review Comment:
👍
##########
common/src/main/java/org/apache/cassandra/sidecar/cluster/InstancesConfigImpl.java:
##########
@@ -85,10 +85,12 @@ public InstanceMetadata instanceFromHost(String host) throws NoSuchElementExcept
}
catch (UnknownHostException e)
{
- throw new NoSuchElementException("Instance with host address "
- + host +
- " not found, and an error occurred when " +
- "attempting to get its IP address.");
+ NoSuchElementException error = new NoSuchElementException("Instance with host address "
+ + host +
+ " not found, and an error occurred when " +
+ "attempting to resolve its IP address.");
+ error.initCause(e);
Review Comment:
👍
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#issuecomment-1654628837
CI running at https://app.circleci.com/pipelines/github/JeetKunDoug/cassandra-sidecar/42/workflows/3430e1c7-1e87-4a35-b263-d7e31b8e49c1
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c closed pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c closed pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
URL: https://github.com/apache/cassandra-sidecar/pull/51
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1278133769
##########
src/test/integration/org/apache/cassandra/testing/ConfigurableCassandraTestContext.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.testing;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+
+import org.apache.cassandra.distributed.UpgradeableCluster;
+
+/**
+ * A Cassandra Test Context implementation that allows advanced cluster configuration before cluster creation
+ * by providing access to the cluster builder.
+ */
+public class ConfigurableCassandraTestContext extends AbstractCassandraTestContext
+{
+ public static final String BUILT_CLUSTER_CANNOT_BE_CONFIGURED_ERROR =
+ "Cannot configure a cluster after it is built. Please set the buildCluster annotation attribute to false, " +
+ "and do not call `getCluster` before calling this method.";
+
+ private final UpgradeableCluster.Builder builder;
+
+ public ConfigurableCassandraTestContext(SimpleCassandraVersion version,
+ UpgradeableCluster.Builder builder)
+ {
+ super(version);
+ this.builder = builder;
+ }
+
+ public UpgradeableCluster configureCluster(Consumer<UpgradeableCluster.Builder> configurator) throws IOException
+ {
+ if (cluster != null)
+ {
+ throw new IllegalStateException(BUILT_CLUSTER_CANNOT_BE_CONFIGURED_ERROR);
+ }
+ configurator.accept(builder);
Review Comment:
👍
##########
src/test/integration/org/apache/cassandra/testing/ConfigurableCassandraTestContext.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.testing;
+
+import java.io.IOException;
+import java.util.function.Consumer;
+
+import org.apache.cassandra.distributed.UpgradeableCluster;
+
+/**
+ * A Cassandra Test Context implementation that allows advanced cluster configuration before cluster creation
+ * by providing access to the cluster builder.
+ */
+public class ConfigurableCassandraTestContext extends AbstractCassandraTestContext
+{
+ public static final String BUILT_CLUSTER_CANNOT_BE_CONFIGURED_ERROR =
+ "Cannot configure a cluster after it is built. Please set the buildCluster annotation attribute to false, " +
+ "and do not call `getCluster` before calling this method.";
+
+ private final UpgradeableCluster.Builder builder;
+
+ public ConfigurableCassandraTestContext(SimpleCassandraVersion version,
+ UpgradeableCluster.Builder builder)
+ {
+ super(version);
+ this.builder = builder;
+ }
+
+ public UpgradeableCluster configureCluster(Consumer<UpgradeableCluster.Builder> configurator) throws IOException
+ {
+ if (cluster != null)
+ {
+ throw new IllegalStateException(BUILT_CLUSTER_CANNOT_BE_CONFIGURED_ERROR);
+ }
+ configurator.accept(builder);
Review Comment:
👍
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#issuecomment-1658438242
Latest CI: https://app.circleci.com/pipelines/github/JeetKunDoug/cassandra-sidecar/48/workflows/536c4ad3-1471-4e5c-bf74-858fd9a6cf94
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#issuecomment-1636102718
Clean CI build:
https://app.circleci.com/pipelines/github/JeetKunDoug/cassandra-sidecar/41/workflows/d2759a94-4443-410c-b012-69a81605f06b
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1279858353
##########
build.gradle:
##########
@@ -72,11 +77,22 @@ allprojects {
excludeFilter = file("${project.rootDir}/spotbugs-exclude.xml")
}
- tasks.withType(com.github.spotbugs.SpotBugsTask) {
+ tasks.withType(SpotBugsTask) {
reports.xml.enabled = false
reports.html.enabled = true
}
+ codeCheckTasks.dependsOn(tasks.withType(Checkstyle))
+ codeCheckTasks.dependsOn(tasks.withType(RatTask))
+ codeCheckTasks.dependsOn(tasks.withType(SpotBugsTask))
+
+ tasks.withType(Test) {
+ shouldRunAfter(codeCheckTasks)
+ shouldRunAfter(tasks.withType(Checkstyle))
+ shouldRunAfter(tasks.withType(RatTask))
+ shouldRunAfter(tasks.withType(SpotBugsTask))
+ }
+
Review Comment:
So, you want to achieve the effect of `check` with `test`? If you run `./gradlew check`, it should run test and the checks specified.
##########
common/src/main/java/org/apache/cassandra/sidecar/cluster/InstancesConfigImpl.java:
##########
@@ -75,7 +79,18 @@ public InstanceMetadata instanceFromHost(String host) throws NoSuchElementExcept
InstanceMetadata instanceMetadata = hostToInstanceMetadata.get(host);
if (instanceMetadata == null)
{
- throw new NoSuchElementException("Instance with host address " + host + " not found");
+ try
+ {
+ instanceMetadata = hostToInstanceMetadata.get(dnsResolver.resolve(host));
+ }
+ catch (UnknownHostException e)
+ {
+ throw new RuntimeException(e);
Review Comment:
nit: Should it throw `NoSuchElementException` in this case too? An unresolved host is very much the same as the instance metadata cannot be found. From API consumer's perspective, it either looks up the instance metadata, or the value does not exist.
##########
adapters/base/build.gradle:
##########
@@ -59,7 +60,7 @@ publishing {
maven(MavenPublication) {
from components.java
groupId project.group
- artifactId "${archivesBaseName}"
+ artifactId "adapters-base"
Review Comment:
Just providing a data point
https://mvnrepository.com/search?q=base
All the artifacts in the first page has `-base` instead of `base-`
##########
.gitignore:
##########
@@ -77,7 +77,7 @@ src/dist/*
*.logdir_IS_UNDEFINED
# Sidecar version - generated by build.gradle
-src/main/resources/sidecar.version
+common/src/main/resources/sidecar.version
Review Comment:
nit: it could be just `sidecar.version`
##########
src/test/integration/org/apache/cassandra/sidecar/testing/CassandraSidecarTestContext.java:
##########
@@ -142,6 +182,8 @@ public String toString()
public void close()
{
instancesConfig.instances().forEach(instance -> instance.delegate().close());
+ // TODO: Do we need to close sessions?
+// sessionProviders.
Review Comment:
It is to be resolved?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] yifan-c commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1276927372
##########
src/test/integration/org/apache/cassandra/testing/CassandraIntegrationTest.java:
##########
@@ -94,4 +96,32 @@
*/
boolean nativeTransport() default true;
+ /**
+ * Return whether the cluster should be started before the test begins.
+ * It may be necessary to delay start/start in a thread if using ByteBuddy-based
+ * interception of cluster startup.
+ * @return true, if the cluster should be started before the test starts, false otherwise
+ */
+ boolean startCluster() default true;
+
+ /**
+ * Return whether the cluster should be built, or to simply add the cluster builder to the context.
+ * This may be useful in cases where the test requires more complex cluster startup.
+ * If false, the test should take an instance of {@link ConfigurableCassandraTestContext}
+ * and call {@link ConfigurableCassandraTestContext#configureCluster(Consumer)}
+ * or {@link ConfigurableCassandraTestContext#configureAndStartCluster(Consumer)} to get the cluster.
+ * NOTE: This cluster object must be closed by the test as the framework doesn't have access to it.
+ * If true (the default), the test should take an instance of {@link CassandraTestContext}
+ * {@link CassandraTestContext#getCluster()} will contain the built cluster.
+ * @return true if the cluster should be built by the test framework, false otherwise
+ */
+ boolean buildCluster() default true;
+
+ /**
+ * If the integration test does not need to be run on each version of Cassandra, set this to false
+ * and it will be run only on the first version specified.
+ * @return true if the test should be run on all tested versions of Cassandra,
+ * false if it should be run on the first version.
+ */
+ boolean versionDependent() default true;
Review Comment:
it makes a lot of sense!
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1278142274
##########
src/test/integration/org/apache/cassandra/sidecar/testing/CassandraSidecarTestContext.java:
##########
@@ -87,8 +122,13 @@ private InstancesConfig buildInstancesConfig(CassandraVersionProvider versionPro
// Use the parent of the first data directory as the staging directory
Path dataDirParentPath = Paths.get(dataDirectories[0]).getParent();
assertThat(dataDirParentPath).isNotNull()
- .exists();
- String uploadsStagingDirectory = dataDirParentPath.resolve("staging").toFile().getAbsolutePath();
+ .exists();
+ Path stagingPath = dataDirParentPath.resolve("staging");
+ String uploadsStagingDirectory = stagingPath.toFile().getAbsolutePath();
+ if (!Files.exists(stagingPath))
+ {
+ Files.createDirectory(stagingPath);
+ }
Review Comment:
similar to `mkdir -p ...`
```suggestion
Files.createDirectories(stagingPath);
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1279934347
##########
build.gradle:
##########
@@ -72,11 +77,22 @@ allprojects {
excludeFilter = file("${project.rootDir}/spotbugs-exclude.xml")
}
- tasks.withType(com.github.spotbugs.SpotBugsTask) {
+ tasks.withType(SpotBugsTask) {
reports.xml.enabled = false
reports.html.enabled = true
}
+ codeCheckTasks.dependsOn(tasks.withType(Checkstyle))
+ codeCheckTasks.dependsOn(tasks.withType(RatTask))
+ codeCheckTasks.dependsOn(tasks.withType(SpotBugsTask))
+
+ tasks.withType(Test) {
+ shouldRunAfter(codeCheckTasks)
+ shouldRunAfter(tasks.withType(Checkstyle))
+ shouldRunAfter(tasks.withType(RatTask))
+ shouldRunAfter(tasks.withType(SpotBugsTask))
+ }
+
Review Comment:
I think it is described in line 59:
> // Force checkstyle, rat, and spotBugs to run before test tasks for faster feedback
Just tried running `./gradlew test` locally (by removing license header from a couple of java files), and it doesn't modify the test behavior.
`./gradlew check` fails fast before running tests.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1280647715
##########
common/src/main/java/org/apache/cassandra/sidecar/cluster/InstancesConfigImpl.java:
##########
@@ -75,7 +77,18 @@ public InstanceMetadata instanceFromHost(String host) throws NoSuchElementExcept
InstanceMetadata instanceMetadata = hostToInstanceMetadata.get(host);
if (instanceMetadata == null)
{
- throw new NoSuchElementException("Instance with host address " + host + " not found");
+ try
+ {
+ instanceMetadata = hostToInstanceMetadata.get(InetAddress.getByName(host).getHostAddress());
+ }
+ catch (UnknownHostException e)
+ {
+ throw new RuntimeException(e);
+ }
Review Comment:
Done
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1277781607
##########
common/src/main/java/org/apache/cassandra/sidecar/cluster/InstancesConfigImpl.java:
##########
@@ -75,7 +77,18 @@ public InstanceMetadata instanceFromHost(String host) throws NoSuchElementExcept
InstanceMetadata instanceMetadata = hostToInstanceMetadata.get(host);
if (instanceMetadata == null)
{
- throw new NoSuchElementException("Instance with host address " + host + " not found");
+ try
+ {
+ instanceMetadata = hostToInstanceMetadata.get(InetAddress.getByName(host).getHostAddress());
+ }
+ catch (UnknownHostException e)
+ {
+ throw new RuntimeException(e);
+ }
Review Comment:
We should use the DnsResolver here. Which means you'll need to pass it to the constructor, or we change the signature of the `instanceFromHost` method to take the DnsResolver. It seems like all the calling points for `instanceFromHost` should have access to the DnsResolver, so we could change the signature
```suggestion
try
{
instanceMetadata = hostToInstanceMetadata.get(dnsResolver.resolve(host));
}
catch (UnknownHostException e)
{
throw new RuntimeException("Unable to resolve instance with host address " + host, e);
}
```
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1280714140
##########
src/test/integration/org/apache/cassandra/sidecar/testing/CassandraSidecarTestContext.java:
##########
@@ -142,6 +182,8 @@ public String toString()
public void close()
{
instancesConfig.instances().forEach(instance -> instance.delegate().close());
+ // TODO: Do we need to close sessions?
+// sessionProviders.
Review Comment:
To be a bit more specific, I'm fairly certain that shutting down the Sidecar server itself will eventually close these, but in case somehow the sidecar server doesn't, it makes sense to close them explicitly here.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] JeetKunDoug commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1280665513
##########
common/src/main/java/org/apache/cassandra/sidecar/cluster/InstancesConfigImpl.java:
##########
@@ -75,7 +79,18 @@ public InstanceMetadata instanceFromHost(String host) throws NoSuchElementExcept
InstanceMetadata instanceMetadata = hostToInstanceMetadata.get(host);
if (instanceMetadata == null)
{
- throw new NoSuchElementException("Instance with host address " + host + " not found");
+ try
+ {
+ instanceMetadata = hostToInstanceMetadata.get(dnsResolver.resolve(host));
+ }
+ catch (UnknownHostException e)
+ {
+ throw new RuntimeException(e);
Review Comment:
So I'd like to be able to include the details of the UnknownHostException, but `NoSuchElementException` doesn't have a constructor that takes a cause as an argument... for now, will just include some additional text in the exception's description.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra-sidecar] frankgh commented on a diff in pull request #51: CASSANDRASC-55: Extract the in-jvm dtest template for use in other projects.
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #51:
URL: https://github.com/apache/cassandra-sidecar/pull/51#discussion_r1279929581
##########
adapters/base/build.gradle:
##########
@@ -59,7 +60,7 @@ publishing {
maven(MavenPublication) {
from components.java
groupId project.group
- artifactId "${archivesBaseName}"
+ artifactId "adapters-base"
Review Comment:
in that case I +1 -base :) . Thanks for the data point
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org