You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by prasadns14 <gi...@git.apache.org> on 2017/10/09 23:28:54 UTC
[GitHub] drill pull request #982: Fixed queued time calculation
GitHub user prasadns14 opened a pull request:
https://github.com/apache/drill/pull/982
Fixed queued time calculation
@paul-rogers please review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/prasadns14/drill DRILL-5716
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/982.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #982
----
commit b5be0b5dd91c73a8a0374bd28b0f1232c2e35e00
Author: Prasad Nagaraj Subramanya <pr...@gmail.com>
Date: 2017-10-03T03:05:05Z
Fixed queued time calculation
----
---
[GitHub] drill issue #982: DRILL-5859: Fixed queued time calculation
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on the issue:
https://github.com/apache/drill/pull/982
Update the title.
---
[GitHub] drill pull request #982: DRILL-5859: Fixed queued time calculation
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/982
---
[GitHub] drill issue #982: Fixed queued time calculation
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/982
Thanks for the first-time commit! We like to have the JIRA number in the title; please go ahead and ecit the title so it looks like this:
DRILL-1234: Fixed queued time calculation
Use the actual Drill JIRA ticket number in place of "1234".
---
[GitHub] drill issue #982: Fixed queued time calculation
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/982
Thanks for changing the commit title. Please also update the pull request title with the JIRA ticket number.
---
[GitHub] drill pull request #982: Fixed queued time calculation
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/982#discussion_r143781752
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -455,9 +455,10 @@ private void admit(QueryWorkUnit work) throws ForemanSetupException {
.build(logger);
} catch (QueryQueueException e) {
throw new ForemanSetupException(e.getMessage(), e);
+ } finally {
+ queryManager.markQueueWaitEndTime();
--- End diff --
Good!
---
[GitHub] drill issue #982: Fixed queued time calculation
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on the issue:
https://github.com/apache/drill/pull/982
@paul-rogers Made the changes as suggested. please review
---
[GitHub] drill pull request #982: Fixed queued time calculation
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/982#discussion_r143781691
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/rm/DistributedQueryQueue.java ---
@@ -273,9 +273,8 @@ public QueueLease enqueue(QueryId queryId, double cost) throws QueryQueueExcepti
}
if (lease == null) {
- int timeoutSecs = (int) Math.round(configSet.queueTimeout/1000.0);
- logger.warn("Queue timeout: {} after {} seconds.", queueName, timeoutSecs);
- throw new QueueTimeoutException(queryId, queueName, timeoutSecs);
+ logger.warn("Queue timeout: {} after {} ms.", queueName, configSet.queueTimeout);
--- End diff --
This was actually not wrong. Instead, it mapped the number from the internal ms units into more user-friendly units of seconds. That is, it is more useful to see:
Queue timeout: small after 60 seconds
Than:
Queue timeout: small after 60000 ms.
If we do want to use ms. perhaps format them with commas:
Queue timeout: small after 60,000 ms.
Or show both:
Queue timeout: small after 60,000 ms. (60 seconds)
---
[GitHub] drill pull request #982: Fixed queued time calculation
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/982#discussion_r143808676
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/rm/EmbeddedQueryQueue.java ---
@@ -93,7 +93,7 @@ public void release() {
public String queueName() { return "local-queue"; }
}
- private final int queueTimeoutMs;
+ private final long queueTimeoutMs;
--- End diff --
Fixed
---
[GitHub] drill pull request #982: Fixed queued time calculation
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/982#discussion_r143808810
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/rm/DistributedQueryQueue.java ---
@@ -273,9 +273,8 @@ public QueueLease enqueue(QueryId queryId, double cost) throws QueryQueueExcepti
}
if (lease == null) {
- int timeoutSecs = (int) Math.round(configSet.queueTimeout/1000.0);
- logger.warn("Queue timeout: {} after {} seconds.", queueName, timeoutSecs);
- throw new QueueTimeoutException(queryId, queueName, timeoutSecs);
+ logger.warn("Queue timeout: {} after {} ms.", queueName, configSet.queueTimeout);
--- End diff --
I made the change to be consistent with QueryTimeoutException which logs the time in ms.
I made the changes as suggested in both the places.
Queue timeout: small after 60,000 ms. (60 seconds)
---
[GitHub] drill pull request #982: Fixed queued time calculation
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/982#discussion_r143781020
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/rm/QueryQueue.java ---
@@ -61,9 +61,9 @@
private final QueryId queryId;
private final String queueName;
- private final int timeoutMs;
+ private final long timeoutMs;
--- End diff --
Same comment as above.
---
[GitHub] drill pull request #982: Fixed queued time calculation
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/982#discussion_r143780986
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/rm/EmbeddedQueryQueue.java ---
@@ -93,7 +93,7 @@ public void release() {
public String queueName() { return "local-queue"; }
}
- private final int queueTimeoutMs;
+ private final long queueTimeoutMs;
--- End diff --
It is not necessary for the timeout to be a long. Timeouts should be on the order of minutes, not decades, so an int is perfectly fine and represents the semantics just a bit better.
---
[GitHub] drill pull request #982: Fixed queued time calculation
Posted by prasadns14 <gi...@git.apache.org>.
Github user prasadns14 commented on a diff in the pull request:
https://github.com/apache/drill/pull/982#discussion_r143808686
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/rm/QueryQueue.java ---
@@ -61,9 +61,9 @@
private final QueryId queryId;
private final String queueName;
- private final int timeoutMs;
+ private final long timeoutMs;
--- End diff --
Fixed
---