frankgh commented on code in PR #118:
URL: https://github.com/apache/cassandra-sidecar/pull/118#discussion_r1586926532
##########
client-common/build.gradle:
##########
@@ -67,8 +67,6 @@ dependencies {
testImplementation("org.junit.jupiter:junit-jupiter-params:${project.junitVersion}")
testImplementation(group: 'io.netty', name: 'netty-codec-http', version:
'4.1.69.Final')
testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:${project.junitVersion}")
-
- testFixturesImplementation("org.assertj:assertj-core:3.24.2")
Review Comment:
the dependency section should look like this I think :
```
dependencies {
compileOnly(group: 'org.slf4j', name: 'slf4j-api', version:
"${project.slf4jVersion}")
compileOnly(group: 'com.fasterxml.jackson.core', name:
'jackson-databind', version: "${project.jacksonVersion}")
compileOnly(group: 'org.jetbrains', name: 'annotations', version:
'23.0.0')
compileOnly(group: 'io.netty', name: 'netty-codec-http', version:
'4.1.69.Final')
compileOnly(group: 'com.google.guava', name: 'guava', version:
"${project.guavaVersion}")
testImplementation(group: 'com.fasterxml.jackson.core', name:
'jackson-databind', version: "${project.jacksonVersion}")
testImplementation("com.google.guava:guava:${project.guavaVersion}")
testImplementation('org.mockito:mockito-inline:4.10.0')
testImplementation("org.assertj:assertj-core:3.24.2")
testImplementation("org.junit.jupiter:junit-jupiter-api:${project.junitVersion}")
testImplementation("org.junit.jupiter:junit-jupiter-params:${project.junitVersion}")
testImplementation(group: 'io.netty', name: 'netty-codec-http', version:
'4.1.69.Final')
testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:${project.junitVersion}")
}
```
##########
client-common/build.gradle:
##########
@@ -44,8 +44,8 @@ test {
maxParallelForks = Runtime.runtime.availableProcessors().intdiv(2) ?: 1
reports {
junitXml.enabled = true
- def destDir = Paths.get(rootProject.rootDir.absolutePath, "build",
"test-results", "common").toFile()
- println("Destination directory for common tests: ${destDir}")
+ def destDir = Paths.get(rootProject.rootDir.absolutePath, "build",
"test-results", "client-common").toFile()
Review Comment:
do we really need to publish client common? I am leaning towards not
publishing it because we directly consume the shaded version.
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableImporter.java:
##########
@@ -165,11 +165,9 @@ private void processPendingImports(Long timerId)
{
if (!queue.isEmpty())
{
+ // schedule task to drain the queue of the cooresponding host
executorPools.internal()
- .executeBlocking(() -> {
- maybeDrainImportQueue(queue);
- return null;
- });
+ .runBlocking(() -> maybeDrainImportQueue(queue));
Review Comment:
👍
##########
src/main/java/org/apache/cassandra/sidecar/concurrent/TaskExecutorPool.java:
##########
@@ -204,6 +225,15 @@ Future<Void> closeInternal()
: workerExecutor.close();
}
+ /**
+ * ThrowableRunnable is similar to {@link Runnable}, but it can throw
{@link Exception}
+ */
+ @FunctionalInterface
+ public interface ThrowableRunnable
+ {
+ void run() throws Exception;
Review Comment:
does it make more sense to throw Throwable given the name of the class?
```suggestion
void run() throws Throwable;
```
##########
src/main/java/org/apache/cassandra/sidecar/restore/RestoreSliceTask.java:
##########
@@ -311,10 +311,7 @@ Future<Void> unzipAndImport(Promise<RestoreSlice> event,
File file, Runnable onS
return Future.succeededFuture();
}
- return executorPool.<Void>executeBlocking(promise -> {
- onSuccessCommit.run();
- promise.tryComplete();
- });
+ return executorPool.runBlocking(onSuccessCommit::run);
Review Comment:
nice
##########
src/main/java/org/apache/cassandra/sidecar/restore/RestoreJobManager.java:
##########
@@ -195,17 +195,13 @@ private Future<Void> deleteDataOfJobAsync(UUID jobId)
{
LOGGER.warn("Error on listing staged restore job directories.
Path={}", stagingDir, ioe);
}
- finally
- {
- promise.complete(); // always completes the promise regardless
of the deletion result
Review Comment:
is this guaranteed now?
--
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]
