You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "cryptoe (via GitHub)" <gi...@apache.org> on 2024/02/26 05:58:22 UTC
[PR] Adding task pending time in MSQ reports (druid)
cryptoe opened a new pull request, #15966:
URL: https://github.com/apache/druid/pull/15966
Added a new field `pendingMs` in MSQ task reports. This helps in figuring out the exact run time of the MSQ worker tasks.
### 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`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe merged PR #15966:
URL: https://github.com/apache/druid/pull/15966
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #15966:
URL: https://github.com/apache/druid/pull/15966#discussion_r1503652111
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -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))
Review Comment:
I personally find 2 not statements a little tricky so hence like the parenthesis. Keeping it as is for 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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #15966:
URL: https://github.com/apache/druid/pull/15966#discussion_r1502383618
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -390,6 +391,12 @@ public long getDuration()
{
return duration;
}
+
+ @JsonProperty("pendingMs")
+ public long getPendingTimeInMS()
Review Comment:
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #15966:
URL: https://github.com/apache/druid/pull/15966#discussion_r1503652111
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -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))
Review Comment:
I personally find 2 not statements a little tricky so hence like the parenthesis. Keeping it as is for.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #15966:
URL: https://github.com/apache/druid/pull/15966#discussion_r1502505249
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -390,6 +391,12 @@ public long getDuration()
{
return duration;
}
+
+ @JsonProperty("pendingMs")
+ public long getPendingTimeInMS()
Review Comment:
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "cryptoe (via GitHub)" <gi...@apache.org>.
cryptoe commented on code in PR #15966:
URL: https://github.com/apache/druid/pull/15966#discussion_r1502383618
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -390,6 +391,12 @@ public long getDuration()
{
return duration;
}
+
+ @JsonProperty("pendingMs")
+ public long getPendingTimeInMS()
Review Comment:
Since we already have `durationMs` as part of line:389 of this payload, have `pendingTimeMillis` would be weird. If you feel strongly, I can change.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "adarshsanjeev (via GitHub)" <gi...@apache.org>.
adarshsanjeev commented on code in PR #15966:
URL: https://github.com/apache/druid/pull/15966#discussion_r1502533384
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -880,17 +899,40 @@ && unknownLocation()
*/
public void enableRetrying()
{
- isRetrying = true;
+ isRetryingRef.set(true);
}
/**
* Checks is the task is retrying,
- *
- * @return
*/
public boolean isRetrying()
{
- return isRetrying;
+ return isRetryingRef.get();
+ }
+
+ public void updateLocation(TaskLocation taskLocation)
Review Comment:
nit: should these be `setLocation`?
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -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))
Review Comment:
nit: this looks a bit weird due to the parenthesis and new lines.
```suggestion
if (!canceledWorkerTasks.contains(taskId) && !tracker.isComplete()) {
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Adding task pending time in MSQ reports (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15966:
URL: https://github.com/apache/druid/pull/15966#discussion_r1502298721
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/indexing/MSQWorkerTaskLauncher.java:
##########
@@ -390,6 +391,12 @@ public long getDuration()
{
return duration;
}
+
+ @JsonProperty("pendingMs")
+ public long getPendingTimeInMS()
Review Comment:
`pendingTimeMillis` would probably be a better name and more in line with rest of the names in Druid.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org