You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by dvjyothsna <gi...@git.apache.org> on 2018/03/30 00:21:26 UTC
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
GitHub user dvjyothsna opened a pull request:
https://github.com/apache/drill/pull/1196
DRILL-6286: Fixed incorrect reference to shutdown in drillbit.log
@vrozov , @ilooner Please review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dvjyothsna/drill DRILL-6286
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1196.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 #1196
----
commit db251cf61e2e07880fd5a6756ccf31c2df60e338
Author: dvjyothsna <ro...@...>
Date: 2018-03-30T00:11:49Z
DRILL-6286: Fixed incorrect reference to shutdown in drillbit.log
----
---
[GitHub] drill issue #1196: DRILL-6286: Fixed incorrect reference to shutdown in dril...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on the issue:
https://github.com/apache/drill/pull/1196
+1
---
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1196#discussion_r178376362
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java ---
@@ -212,19 +218,29 @@ private boolean areQueriesAndFragmentsEmpty() {
return queries.isEmpty() && runningFragments.isEmpty();
}
+ /**
+ * Check if there any new queries or fragments that are added after the shutdown is triggered
+ */
+ private boolean areNewQueriesOrFragmentsAdded() {
+ return runningFragments.size() > numOfRunningFragments || queries.size() > numOfRunningQueries;
+ }
+
/**
* A thread calling the {@link #waitToExit(boolean)} method is notified when a foreman is retired.
*/
private void indicateIfSafeToExit() {
isEmptyLock.lock();
try {
- logger.info("Waiting for "+ queries.size() +" queries to complete before shutting down");
- logger.info("Waiting for "+ runningFragments.size() +" running fragments to complete before shutting down");
+ if (isShutdownTriggered) {
+ logger.info("Waiting for "+ queries.size() +" queries to complete before shutting down");
--- End diff --
Use slf4j smart logging.
---
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1196#discussion_r178376213
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java ---
@@ -212,19 +218,29 @@ private boolean areQueriesAndFragmentsEmpty() {
return queries.isEmpty() && runningFragments.isEmpty();
}
+ /**
+ * Check if there any new queries or fragments that are added after the shutdown is triggered
+ */
+ private boolean areNewQueriesOrFragmentsAdded() {
+ return runningFragments.size() > numOfRunningFragments || queries.size() > numOfRunningQueries;
--- End diff --
This condition is not reliable. What if some fragments exited and some were added? The total number may still be less than the number of fragments when the shutdown was requested.
---
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1196#discussion_r178375583
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java ---
@@ -86,6 +86,9 @@
private final StatusThread statusThread;
private final Lock isEmptyLock = new ReentrantLock();
private final Condition isEmptyCondition = isEmptyLock.newCondition();
+ private boolean isShutdownTriggered = false;
--- End diff --
Is this boolean necessary? Can you delay getting `isEmptyCondition` till shutdown is requested and use it in place of `isShutdownTriggered`?
---
[GitHub] drill pull request #1196: DRILL-6286: Fixed incorrect reference to shutdown ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1196
---
[GitHub] drill issue #1196: DRILL-6286: Fixed incorrect reference to shutdown in dril...
Posted by dvjyothsna <gi...@git.apache.org>.
Github user dvjyothsna commented on the issue:
https://github.com/apache/drill/pull/1196
@vrozov please review
---