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 2024/03/07 20:59:49 UTC
[PR] CASSANDRASC-106: Add restore task watcher to report long running tasks [cassandra-sidecar]
JeetKunDoug opened a new pull request, #104:
URL: https://github.com/apache/cassandra-sidecar/pull/104
(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-106: Add restore task watcher to report long running tasks [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #104:
URL: https://github.com/apache/cassandra-sidecar/pull/104#discussion_r1525605975
##########
src/main/java/org/apache/cassandra/sidecar/restore/RestoreSliceTask.java:
##########
@@ -236,6 +242,11 @@ else if (s3Exception.statusCode() == 400 &&
});
}
+ private long currentTimeInNanos()
Review Comment:
missing `@Override` annotations
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/RestoreJobConfigurationImpl.java:
##########
@@ -36,6 +36,10 @@ public class RestoreJobConfigurationImpl implements RestoreJobConfiguration
public static final int DEFAULT_JOB_DISCOVERY_RECENCY_DAYS = 5;
public static final int DEFAULT_PROCESS_MAX_CONCURRENCY = 20; // process at most 20 slices concurrently
public static final long DEFAULT_RESTORE_JOB_TABLES_TTL_SECONDS = TimeUnit.DAYS.toSeconds(90);
+ public static final String RESTORE_JOB_LONG_RUNNING_HANDLER_THRESHOLD_SECONDS =
+ "restore_job_long_running_threshold_seconds";
+ // A restore job handler is considered long-running if it has been in the "active" list for 10 minutes.
+ private static final long DEFAULT_RESTORE_JOB_LONG_RUNNING_HANDLER_THRESHOLD_SECONDS = 600;
Review Comment:
Let's use TimeUnit to define the time for consistency and accuracy
```suggestion
private static final long DEFAULT_RESTORE_JOB_LONG_RUNNING_HANDLER_THRESHOLD_SECONDS = TimeUnit.MINUTES.toSeconds(10);
```
##########
src/main/java/org/apache/cassandra/sidecar/restore/RestoreSliceTask.java:
##########
@@ -513,4 +524,48 @@ private void logWarnIfHasHttpExceptionCauseOnCommit(Throwable throwable, Restore
LOGGER.warn("Committing slice failed with HttpException. slice={} statusCode={} exceptionPayload={}",
slice.sliceId(), httpException.getStatusCode(), httpException.getPayload(), httpException);
}
+
+ public long elapsedInNanos()
+ {
+ return taskStartTimeNanos == -1 ? -1 :
+ currentTimeInNanos() - taskStartTimeNanos;
+ }
+
+ public RestoreSlice slice()
+ {
+ return slice;
+ }
+
+ /**
+ * A RestoreSliceHandler that immediately fails the slice/promise.
+ * Used when the processor already knows that a slice should not be processed for some reason
+ * as indicated in cause field.
+ */
+ public static class Failed implements RestoreSliceHandler
+ {
+ private final RestoreJobException cause;
+ private final RestoreSlice slice;
+
+ public Failed(RestoreJobException cause, RestoreSlice slice)
+ {
+ this.cause = cause;
+ this.slice = slice;
+ }
+
+ public void handle(Promise<RestoreSlice> promise)
Review Comment:
can we add missing `@Override` annotations here?
##########
src/test/java/org/apache/cassandra/sidecar/stats/TestRestoreJobStats.java:
##########
@@ -118,4 +121,9 @@ public void captureTokenRefreshed()
{
tokenRefreshCount += 1;
}
+
+ public void captureLongRunningRestoreHandler(int instanceId, long handlerDuration)
Review Comment:
NIT missing`@Override` tag
--
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-106: Add restore task watcher to report long running tasks [cassandra-sidecar]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #104:
URL: https://github.com/apache/cassandra-sidecar/pull/104#discussion_r1532862150
##########
src/main/java/org/apache/cassandra/sidecar/restore/RestoreSliceTask.java:
##########
@@ -513,4 +524,48 @@ private void logWarnIfHasHttpExceptionCauseOnCommit(Throwable throwable, Restore
LOGGER.warn("Committing slice failed with HttpException. slice={} statusCode={} exceptionPayload={}",
slice.sliceId(), httpException.getStatusCode(), httpException.getPayload(), httpException);
}
+
+ public long elapsedInNanos()
+ {
+ return taskStartTimeNanos == -1 ? -1 :
+ currentTimeInNanos() - taskStartTimeNanos;
+ }
+
+ public RestoreSlice slice()
+ {
+ return slice;
+ }
+
+ /**
+ * A RestoreSliceHandler that immediately fails the slice/promise.
+ * Used when the processor already knows that a slice should not be processed for some reason
+ * as indicated in cause field.
+ */
+ public static class Failed implements RestoreSliceHandler
+ {
+ private final RestoreJobException cause;
+ private final RestoreSlice slice;
+
+ public Failed(RestoreJobException cause, RestoreSlice slice)
+ {
+ this.cause = cause;
+ this.slice = slice;
+ }
+
+ public void handle(Promise<RestoreSlice> promise)
Review Comment:
I'm doing a global "fix all the missing `@Override` annotations" on the project as part of this PR - there were 30, and rather than piecemeal fixing them as we go I figure I'd just fix them all :)
--
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-106: Add restore task watcher to report long running tasks [cassandra-sidecar]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #104:
URL: https://github.com/apache/cassandra-sidecar/pull/104#discussion_r1532872954
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/RestoreJobConfigurationImpl.java:
##########
@@ -36,6 +36,10 @@ public class RestoreJobConfigurationImpl implements RestoreJobConfiguration
public static final int DEFAULT_JOB_DISCOVERY_RECENCY_DAYS = 5;
public static final int DEFAULT_PROCESS_MAX_CONCURRENCY = 20; // process at most 20 slices concurrently
public static final long DEFAULT_RESTORE_JOB_TABLES_TTL_SECONDS = TimeUnit.DAYS.toSeconds(90);
+ public static final String RESTORE_JOB_LONG_RUNNING_HANDLER_THRESHOLD_SECONDS =
+ "restore_job_long_running_threshold_seconds";
+ // A restore job handler is considered long-running if it has been in the "active" list for 10 minutes.
+ private static final long DEFAULT_RESTORE_JOB_LONG_RUNNING_HANDLER_THRESHOLD_SECONDS = 600;
Review Comment:
So I tried this while implementing it, but it ends up that using a non-constant value means you can't use it as the default value as part of the @JsonProperty annotations... so I'd end up having to put `"600"` in the `@JsonProperty` if we used `TimeUnit.MINUTES.toSeconds(10)` 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
Re: [PR] CASSANDRASC-106: Add restore task watcher to report long running tasks [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh merged PR #104:
URL: https://github.com/apache/cassandra-sidecar/pull/104
--
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