You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "yifan-c (via GitHub)" <gi...@apache.org> on 2024/05/24 20:59:28 UTC
[PR] CASSANDRASC-133: Allow optional reason to abort restore jobs [cassandra-sidecar]
yifan-c opened a new pull request, #124:
URL: https://github.com/apache/cassandra-sidecar/pull/124
(no 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
Re: [PR] CASSANDRASC-133: Allow optional reason to abort restore jobs [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #124:
URL: https://github.com/apache/cassandra-sidecar/pull/124#discussion_r1614029676
##########
src/main/java/org/apache/cassandra/sidecar/routes/restore/AbortRestoreJobHandler.java:
##########
@@ -69,33 +73,47 @@ protected void handleInternal(RoutingContext context,
HttpServerRequest httpRequest,
String host,
SocketAddress remoteAddress,
- String jobId)
+ AbortRestoreJobRequestPayload payload)
{
RoutingContextUtils
.getAsFuture(context, SC_RESTORE_JOB)
- .compose(job -> {
+ .map(job -> {
if (RestoreJobStatus.isFinalState(job.status))
{
- return Future.failedFuture(wrapHttpException(HttpResponseStatus.CONFLICT,
- "Job is already in final state: " + job.status));
+ throw wrapHttpException(HttpResponseStatus.CONFLICT,
+ "Job is already in final state: " + job.status);
}
- restoreJobDatabaseAccessor.abort(job.jobId);
+ restoreJobDatabaseAccessor.abort(job.jobId, payload.reason());
+ logger.info("Successfully aborted restore job. job={} remoteAddress={} instance={} reason='{}'",
+ job, remoteAddress, host, payload.reason());
restoreJobManagerGroup.signalRefreshRestoreJob();
- return Future.succeededFuture(job);
+ return job;
})
.onSuccess(job -> {
- logger.info("Successfully aborted restore job. job={}, remoteAddress={}, instance={}",
- job, remoteAddress, host);
metrics.failedJobs.metric.update(1);
context.response().setStatusCode(HttpResponseStatus.OK.code()).end();
})
- .onFailure(cause -> processFailure(cause, context, host, remoteAddress, jobId));
+ .onFailure(cause -> processFailure(cause, context, host, remoteAddress, payload));
}
+ @NotNull
@Override
- protected String extractParamsOrThrow(RoutingContext context)
+ protected AbortRestoreJobRequestPayload extractParamsOrThrow(RoutingContext context)
{
- return context.pathParam("jobId");
+ String bodyString = context.body().asString(); // nullable
+
+ try
+ {
+ return Json.decodeValue(bodyString, AbortRestoreJobRequestPayload.class);
+ }
+ catch (Exception anyException)
Review Comment:
maybe just call it exception? I feel this is introducing a different style that differs from the existing codebase
```suggestion
catch (Exception exception)
```
##########
src/main/java/org/apache/cassandra/sidecar/routes/restore/AbortRestoreJobHandler.java:
##########
@@ -69,33 +73,47 @@ protected void handleInternal(RoutingContext context,
HttpServerRequest httpRequest,
String host,
SocketAddress remoteAddress,
- String jobId)
+ AbortRestoreJobRequestPayload payload)
{
RoutingContextUtils
.getAsFuture(context, SC_RESTORE_JOB)
- .compose(job -> {
+ .map(job -> {
if (RestoreJobStatus.isFinalState(job.status))
{
- return Future.failedFuture(wrapHttpException(HttpResponseStatus.CONFLICT,
- "Job is already in final state: " + job.status));
+ throw wrapHttpException(HttpResponseStatus.CONFLICT,
+ "Job is already in final state: " + job.status);
}
- restoreJobDatabaseAccessor.abort(job.jobId);
+ restoreJobDatabaseAccessor.abort(job.jobId, payload.reason());
+ logger.info("Successfully aborted restore job. job={} remoteAddress={} instance={} reason='{}'",
+ job, remoteAddress, host, payload.reason());
restoreJobManagerGroup.signalRefreshRestoreJob();
- return Future.succeededFuture(job);
+ return job;
})
.onSuccess(job -> {
- logger.info("Successfully aborted restore job. job={}, remoteAddress={}, instance={}",
- job, remoteAddress, host);
metrics.failedJobs.metric.update(1);
context.response().setStatusCode(HttpResponseStatus.OK.code()).end();
})
- .onFailure(cause -> processFailure(cause, context, host, remoteAddress, jobId));
+ .onFailure(cause -> processFailure(cause, context, host, remoteAddress, payload));
}
+ @NotNull
@Override
- protected String extractParamsOrThrow(RoutingContext context)
+ protected AbortRestoreJobRequestPayload extractParamsOrThrow(RoutingContext context)
{
- return context.pathParam("jobId");
+ String bodyString = context.body().asString(); // nullable
+
+ try
+ {
+ return Json.decodeValue(bodyString, AbortRestoreJobRequestPayload.class);
+ }
+ catch (Exception anyException)
+ {
+ if (bodyString != null)
+ {
+ logger.warn("Failed to deserialize json string into AbortRestoreJobRequestPayload", anyException);
Review Comment:
Remove extra space here
```suggestion
logger.warn("Failed to deserialize json string into AbortRestoreJobRequestPayload", anyException);
```
##########
client/src/main/java/org/apache/cassandra/sidecar/client/SidecarClient.java:
##########
@@ -517,10 +518,11 @@ public CompletableFuture<Void> updateRestoreJob(String keyspace,
* {@inheritDoc}
*/
@Override
- public CompletableFuture<Void> abortRestoreJob(String keyspace, String table, UUID jobId)
+ public CompletableFuture<Void> abortRestoreJob(String keyspace, String table, UUID jobId,
Review Comment:
I feel we should preserve the existing signature and add another method that takes payload. For the existing signature, the payload would be null.
--
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
Re: [PR] CASSANDRASC-133: Allow optional reason to abort restore jobs [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c merged PR #124:
URL: https://github.com/apache/cassandra-sidecar/pull/124
--
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