bbotella commented on code in PR #123:
URL: https://github.com/apache/cassandra-sidecar/pull/123#discussion_r1613473604
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/data/RestoreJobConstants.java:
##########
@@ -48,4 +49,12 @@ public class RestoreJobConstants
public static final String CREDENTIALS_SECRET_ACCESS_KEY =
"secretAccessKey";
public static final String CREDENTIALS_SESSION_TOKEN = "sessionToken";
public static final String CREDENTIALS_REGION = "region";
+
+ // -- restore job status fields
+ public static final String JOB_STATUS_MESSAGE = "message";
+ public static final String JOB_STATUS_SUMMARY = "summary";
+ public static final String JOB_STATUS_FAILED_RANGE = "failedRanges";
+ public static final String JOB_STATUS_ABORTED_RANGE = "abortedRanges";
+ public static final String JOB_STATUS_PENDING_RANGE = "pendingRanges";
+ public static final String JOB_STATUS_SUCCEEDED_RANGE = "succeededRanges";
Review Comment:
Nit: Should we end the const names as plurals? `*_RANGES`
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/utils/Preconditions.java:
##########
@@ -32,10 +34,29 @@ public class Preconditions
* @throws IllegalArgumentException when the condition is not valid (i.e.
{@code false})
*/
public static void checkArgument(boolean validCondition, String
errorMessage)
+ {
+ expect(validCondition, () -> new
IllegalArgumentException(errorMessage));
+ }
+
+ /**
+ * Validate the condition and throw the supplied exception if invalid.
+ *
+ * @param validCondition the condition to evaluate
+ * @param exceptionSupplier supplies the {@link RuntimeException} to be
thrown when condition is invalid
+ */
+ public static void expect(boolean validCondition,
Supplier<RuntimeException> exceptionSupplier)
{
if (!validCondition)
{
- throw new IllegalArgumentException(errorMessage);
+ throw exceptionSupplier.get();
+ }
+ }
+
+ public static void throwsOn(boolean invalidCondition,
Supplier<RuntimeException> exceptionSupplier)
Review Comment:
Javadoc?
##########
server-common/src/main/java/org/apache/cassandra/sidecar/common/server/CQLSessionProvider.java:
##########
@@ -31,23 +31,24 @@
public interface CQLSessionProvider
{
/**
- * Provides a Session connected to the cluster. If null it means the
connection was
- * could not be established. The session still might throw a
NoHostAvailableException if the
- * cluster is otherwise unreachable.
+ * Provides a Session connected to the cluster; otherwise, it tries to
connect to the cluster.
+ * Returning null means the connection can not be established.
Review Comment:
Why null? Would it we worth to throw a different exception? Something like
`ConnectionCouldNotBeStablishedException`?
##########
server-common/src/test/java/org/apache/cassandra/sidecar/common/server/utils/ThrowableUtilsTest.java:
##########
@@ -69,4 +69,10 @@ void testGetCauseWithPredicate()
&&
cause.getMessage().equals("non-existing exception")))
.isNull();
}
+
+ @Test
+ void testPropogate()
Review Comment:
Test logic missing
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/RestoreJobConfigurationImpl.java:
##########
@@ -56,11 +55,12 @@ public class RestoreJobConfigurationImpl implements
RestoreJobConfiguration
@JsonProperty(value = "restore_job_tables_ttl_seconds")
protected final long restoreJobTablesTtlSeconds;
-
- @JsonProperty(value = RESTORE_JOB_LONG_RUNNING_HANDLER_THRESHOLD_SECONDS,
- defaultValue = DEFAULT_RESTORE_JOB_LONG_RUNNING_HANDLER_THRESHOLD_SECONDS
+ "")
+ @JsonProperty(value = "restore_job_long_running_threshold_seconds")
Review Comment:
Why are we removing the constant to hardcode the string?
##########
server-common/src/main/java/org/apache/cassandra/sidecar/common/server/cluster/locator/TokenRange.java:
##########
@@ -75,18 +76,62 @@ public TokenRange(long start, long end)
public TokenRange(BigInteger start, BigInteger end)
{
- this.start = start;
- this.end = end;
+ this(Token.from(start), Token.from(end));
+ }
+
+ public TokenRange(Token start, Token end)
+ {
+ this.range = Range.openClosed(start, end);
+ }
+
+ /**
+ * @return start token. It is not enclosed in the range.
+ */
+ public Token start()
+ {
+ return range.lowerEndpoint();
}
- public BigInteger start()
+ /**
+ * @return end token. It is the last token enclosed in the range.
+ */
+ public Token end()
{
- return this.start;
+ return range.upperEndpoint();
}
- public BigInteger end()
+ /**
+ * @return the first token enclosed in the range
+ */
+ public Token firstToken()
+ {
+ return range.lowerEndpoint().increment();
+ }
+
+ public boolean encloses(TokenRange other)
Review Comment:
Add some javadocs here?
##########
client-common/src/main/java/org/apache/cassandra/sidecar/common/request/data/CreateRestoreJobRequestPayload.java:
##########
@@ -205,31 +226,48 @@ public static class Builder
public Builder jobId(UUID jobId)
{
- this.jobId = jobId;
- return this;
+ return update(b -> b.jobId = jobId);
}
public Builder jobAgent(String jobAgent)
{
- this.jobAgent = jobAgent;
- return this;
+ return update(b -> b.jobAgent = jobAgent);
}
public Builder updateImportOptions(Consumer<SSTableImportOptions>
updater)
{
- updater.accept(importOptions);
- return this;
+ return update(b -> updater.accept(b.importOptions));
}
- public Builder consistencyLevel(String consistencyLevel)
+ public Builder consistencyLevel(ConsistencyLevel consistencyLevel)
+ {
+ return consistencyLevel(consistencyLevel, null);
+ }
+
+ public Builder consistencyLevel(ConsistencyLevel consistencyLevel,
String localDc)
+ {
+ return update(b -> {
+ b.consistencyLevel = consistencyLevel;
+ b.localDc = localDc;
+ });
+ }
+
+ @Override
+ public Builder self()
{
- this.consistencyLevel = consistencyLevel;
return this;
}
public CreateRestoreJobRequestPayload build()
{
+ Preconditions.checkArgument(!consistencyLevel.isLocalDcOnly ||
(localDc != null && !localDc.isEmpty()),
+ "Must specify a non-empty localDc for
consistency level: " + consistencyLevel);
Review Comment:
Nit: `"Must specify a non-empty " + JOB_LOCAL_DATA_CENTER + " for
consistency level: "`?
--
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]
