-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding task pending time in MSQ reports #15966
Conversation
@@ -390,6 +391,12 @@ public long getDuration() | |||
{ | |||
return duration; | |||
} | |||
|
|||
@JsonProperty("pendingMs") | |||
public long getPendingTimeInMS() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pendingTimeMillis
would probably be a better name and more in line with rest of the names in Druid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already have durationMs
as part of line:389 of this payload, having pendingTimeMillis
would be weird. If you feel strongly about it, I can change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using millis as convention in this class, so it's a bit unfortunate that we already have "durationMs" in the taskReport. Would the best thing be to call it pendingTimeMillis
in the code and expose it in the report as getPendingTimeInMs
. As a nit, I think it should be getPendingTimeInMs
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, minor comments
...ore/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java
Outdated
Show resolved
Hide resolved
@@ -737,8 +750,9 @@ private void shutDownTasks() | |||
for (final Map.Entry<String, TaskTracker> taskEntry : taskTrackers.entrySet()) { | |||
final String taskId = taskEntry.getKey(); | |||
final TaskTracker tracker = taskEntry.getValue(); | |||
if (!canceledWorkerTasks.contains(taskId) | |||
&& (tracker.status == null || !tracker.status.getStatusCode().isComplete())) { | |||
if ((!canceledWorkerTasks.contains(taskId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this looks a bit weird due to the parenthesis and new lines.
if ((!canceledWorkerTasks.contains(taskId)) | |
if (!canceledWorkerTasks.contains(taskId) && !tracker.isComplete()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find 2 not statements a little tricky so hence like the parenthesis. Keeping it as is for now.
pendingMs
in MSQ task reports. This helps in figuring out the exact run time of the MSQ worker tasks.The payload now looks like
Release notes
A new field in MSQ task report which captures the milliseconds elapsed between when the worker task was first requested and when it fully started RUNNING. Actual work time can be calculated using
actualWorkTimeMS = durationMs - pendingMs