You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by hnfgns <gi...@git.apache.org> on 2015/12/22 22:17:03 UTC
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
GitHub user hnfgns opened a pull request:
https://github.com/apache/drill/pull/310
DRILL-4187: introduce a new query state ENQUEUED and rename the state PENDING to STARTING
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hnfgns/incubator-drill DRILL-4187
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/310.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 #310
----
commit f108fcf65078d3173c0c4433494ca0d3f4f85de2
Author: Hanifi Gunes <ha...@gmail.com>
Date: 2015-12-16T01:24:27Z
DRILL-4187: introduce a new query state ENQUEUED and rename the state PENDING to STARTING
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/310#discussion_r48306314
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -458,49 +467,46 @@ private void setupSortMemoryAllocations(final PhysicalPlan plan) {
*/
private void acquireQuerySemaphore(final PhysicalPlan plan) throws ForemanSetupException {
final OptionManager optionManager = queryContext.getOptions();
- final boolean queuingEnabled = optionManager.getOption(ExecConstants.ENABLE_QUEUE);
- if (queuingEnabled) {
- final long queueThreshold = optionManager.getOption(ExecConstants.QUEUE_THRESHOLD_SIZE);
- double totalCost = 0;
- for (final PhysicalOperator ops : plan.getSortedOperators()) {
- totalCost += ops.getCost();
- }
-
- final long queueTimeout = optionManager.getOption(ExecConstants.QUEUE_TIMEOUT);
- final String queueName;
-
- try {
- @SuppressWarnings("resource")
- final ClusterCoordinator clusterCoordinator = drillbitContext.getClusterCoordinator();
- final DistributedSemaphore distributedSemaphore;
-
- // get the appropriate semaphore
- if (totalCost > queueThreshold) {
- final int largeQueue = (int) optionManager.getOption(ExecConstants.LARGE_QUEUE_SIZE);
- distributedSemaphore = clusterCoordinator.getSemaphore("query.large", largeQueue);
- queueName = "large";
- } else {
- final int smallQueue = (int) optionManager.getOption(ExecConstants.SMALL_QUEUE_SIZE);
- distributedSemaphore = clusterCoordinator.getSemaphore("query.small", smallQueue);
- queueName = "small";
- }
+ final long queueThreshold = optionManager.getOption(ExecConstants.QUEUE_THRESHOLD_SIZE);
+ double totalCost = 0;
+ for (final PhysicalOperator ops : plan.getSortedOperators()) {
+ totalCost += ops.getCost();
+ }
+ final long queueTimeout = optionManager.getOption(ExecConstants.QUEUE_TIMEOUT);
+ final String queueName;
- lease = distributedSemaphore.acquire(queueTimeout, TimeUnit.MILLISECONDS);
- } catch (final Exception e) {
- throw new ForemanSetupException("Unable to acquire slot for query.", e);
+ try {
+ @SuppressWarnings("resource")
+ final ClusterCoordinator clusterCoordinator = drillbitContext.getClusterCoordinator();
+ final DistributedSemaphore distributedSemaphore;
+
+ // get the appropriate semaphore
+ if (totalCost > queueThreshold) {
+ final int largeQueue = (int) optionManager.getOption(ExecConstants.LARGE_QUEUE_SIZE);
+ distributedSemaphore = clusterCoordinator.getSemaphore("query.large", largeQueue);
+ queueName = "large";
+ } else {
+ final int smallQueue = (int) optionManager.getOption(ExecConstants.SMALL_QUEUE_SIZE);
+ distributedSemaphore = clusterCoordinator.getSemaphore("query.small", smallQueue);
+ queueName = "small";
}
- if (lease == null) {
- throw UserException
- .resourceError()
- .message(
- "Unable to acquire queue resources for query within timeout. Timeout for %s queue was set at %d seconds.",
- queueName, queueTimeout / 1000)
- .build(logger);
- }
+ recordNewState(QueryState.ENQUEUED);
--- End diff --
Since the current "state" is ENQUEUED, is this required?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/310#issuecomment-166765293
Yes. cancellation remains the same, but it would be nice to be able to cancel when the state is ENQUEUED (interrupt the thread waiting on the semaphore).
+ 1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on the pull request:
https://github.com/apache/drill/pull/310#issuecomment-166731599
@sudheeshkatkam please review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the pull request:
https://github.com/apache/drill/pull/310#issuecomment-166754159
How should cancellation be handled when the query is ENQUEUED?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on the pull request:
https://github.com/apache/drill/pull/310#issuecomment-166762090
Cancellation is handled usual way after query is cancelled only after it has been started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/310
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by hnfgns <gi...@git.apache.org>.
Github user hnfgns commented on the pull request:
https://github.com/apache/drill/pull/310#issuecomment-166762114
Addressed review comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/310#discussion_r48306322
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java ---
@@ -287,6 +288,7 @@ QueryState updateEphemeralState(final QueryState queryState) {
case CANCELED:
case FAILED:
try {
+ profilePStore.put(stringQueryId, getQueryProfile());
--- End diff --
Should be removed. Foreman.ForemanResult#close has the call to write the final profile.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/310#discussion_r48306302
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -135,6 +135,8 @@
private final ForemanResult foremanResult = new ForemanResult();
private final ConnectionClosedListener closeListener = new ConnectionClosedListener();
private final ChannelFuture closeFuture;
+ final boolean queuingEnabled;
--- End diff --
private
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4187: introduce a new query state ENQUEU...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/310#discussion_r48306306
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -391,7 +397,10 @@ private void parseAndRunPhysicalPlan(final String json) throws ExecutionSetupExc
private void runPhysicalPlan(final PhysicalPlan plan) throws ExecutionSetupException {
validatePlan(plan);
setupSortMemoryAllocations(plan);
- acquireQuerySemaphore(plan);
+ if (queuingEnabled) {
+ acquireQuerySemaphore(plan);
+ }
+ recordNewState(QueryState.STARTING);
--- End diff --
If the initialState is STARTING won't this fail (state transition from STARTING to STARTING is invalid given the fall-through)? The recordNewState(STARTING) should be inside the if condition, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---