You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/06/24 06:11:18 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
abhishekagarwal87 opened a new pull request #11382:
URL: https://github.com/apache/druid/pull/11382
### Description
This PR refactors the code for `QueryRunnerFactory#mergeRunners` to accept a new interface called `QueryProcessingPool` instead of `ExecutorService` for concurrent execution of query runners. This interface will let custom extensions inject their own implementation for deciding which query-runner to prioritize first. The default implementation is the same as today that takes the priority of query into account. `QueryProcessingPool` can also be used as a regular executor service. It has a dedicated method for accepting query execution work so implementations can differentiate between regular async tasks and query execution tasks. This dedicated method also passes the `QueryRunner` object as part of the task information. This hook will let custom extensions carry any state from `QuerySegmentWalker` to `QueryProcessingPool#mergeRunners` which is not possible currently.
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
<hr>
##### Key changed/added classes in this PR
* `QueryProcessingPool`
* `QueryRunnerFactory`
* `DefaultQueryProcessingPool`
* `DirectQueryProcessingPool`
* `PrioritizedQueryRunnerCallable`
* `AbstractPrioritizedQueryRunnerCallable`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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.
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
[GitHub] [druid] jihoonson commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658998079
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
Just clarifying my stance here, @rohangarg's idea sounds good to me, but I don't have strong preference here.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r660412749
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
On real-time nodes too, the pool is used in query execution.
--
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
[GitHub] [druid] jon-wei commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r659063606
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
Review comment:
```suggestion
* An implementation of {@link PrioritizedCallable} that also lets caller get access to associated {@link QueryRunner}
```
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
I think making the processing a pool a kind of executor service makes sense, let's go with that if you agree
--
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
[GitHub] [druid] jihoonson commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658128272
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
+ * It is used in implementations of {@link QueryRunnerFactory}
+ * @param <T>
+ * @param <V>
*/
-@BindingAnnotation
-@Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD})
-@Retention(RetentionPolicy.RUNTIME)
-public @interface Processing
+public interface PrioritizedQueryRunnerCallable<T, V> extends PrioritizedCallable<T>
{
+ QueryRunner<V> getRunner();
Review comment:
How will this method be used? I only see it used in a test in this PR.
--
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.
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r659735297
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
+ * It is used in implementations of {@link QueryRunnerFactory}
+ * @param <T>
+ * @param <V>
Review comment:
done in next patch.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658553705
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
something like this?
```
@ExtensionPoint
public interface QueryProcessingPool extends ListeningExecutorService
{
/**
* Submits the query execution unit task for asynchronous execution.
*
* @param task - Task to be submitted.
* @param <T> - Task result type
* @param <V> - Query runner sequence type
* @return - Future object for tracking the task completion.
*/
<T, V> ListenableFuture<T> submitQueryExecution(PrioritizedQueryRunnerCallable<T, V> task);
}
```
--
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.
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
[GitHub] [druid] jihoonson commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r661088780
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
SGTM
--
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
[GitHub] [druid] rohangarg commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
rohangarg commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r659723121
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
LGTM
<nit> maybe the naming could be more generic since this would be used in both ingestion and querying layer both.
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658549035
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
+ * It is used in implementations of {@link QueryRunnerFactory}
+ * @param <T>
+ * @param <V>
*/
-@BindingAnnotation
-@Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD})
-@Retention(RetentionPolicy.RUNTIME)
-public @interface Processing
+public interface PrioritizedQueryRunnerCallable<T, V> extends PrioritizedCallable<T>
{
+ QueryRunner<V> getRunner();
Review comment:
This method can be used by the extensions to get the runner that the given query execution task corresponds to. That in turn can be used to fetch any state associated with the QueryRunner such as the segment info for example.
--
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.
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r659264335
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
I do want to keep the method separate though. Either that or `PrioritizedQueryRunnerCallable<T, V> task` should not extend `Callable`. My reasoning is that then Implementations don't have to do `instance of` checks for differentiating between query execution tasks and other async tasks.
Yes
> I assume you want to make QueryProcessingPool compatible with ExecutorService because of ConcurrentGrouper?
--
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
[GitHub] [druid] rohangarg commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
rohangarg commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658681554
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
Yes, I was thinking like the snippet you mentioned above. (probably the interface can be called something like `QueryRunnerProcessingPool` since it only allows submit for `QueryRunners` currently - the `submitQueryExecution` can also become `submit`).
If this interface looks ok, then the DefaultProcessingPool's constructor signature can be : `DefaultProcessingPool(@Processing ExecutorService)`
--
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.
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r660412749
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
On real-time nodes too, the pool is used in query execution.
--
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
[GitHub] [druid] jihoonson commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658883324
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
+ * It is used in implementations of {@link QueryRunnerFactory}
+ * @param <T>
+ * @param <V>
*/
-@BindingAnnotation
-@Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD})
-@Retention(RetentionPolicy.RUNTIME)
-public @interface Processing
+public interface PrioritizedQueryRunnerCallable<T, V> extends PrioritizedCallable<T>
{
+ QueryRunner<V> getRunner();
Review comment:
I see. Can you add it in the javadoc of this method?
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
Review comment:
```suggestion
* An implementation of {@link PrioritizedCallable} that also let caller get access to associated {@link QueryRunner}
```
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
I assume you want to make `QueryProcessingPool` compatible with `ExecutorService` because of `ConcurrentGrouper`?
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
+ * It is used in implementations of {@link QueryRunnerFactory}
+ * @param <T>
+ * @param <V>
Review comment:
Can you please complete the javadoc for the parameters?
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r659264335
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
Sounds good. I do want to keep the method separate though. Either that or `PrioritizedQueryRunnerCallable<T, V> task` should not extend `Callable`. My reasoning is that then Implementations don't have to do `instance of` checks for differentiating between query execution tasks and other async tasks.
Yes
> I assume you want to make QueryProcessingPool compatible with ExecutorService because of ConcurrentGrouper?
--
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
[GitHub] [druid] abhishekagarwal87 merged pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #11382:
URL: https://github.com/apache/druid/pull/11382
--
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
[GitHub] [druid] rohangarg commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
rohangarg commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658063054
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
I think this method leaks the internals of the interface. For instance, some caller may call this method and then pass the executor in other parts of code unintentionally. Also, if someone has to implement a `QueryProcessingPool` which is composite and contains multiple pools inside it, it would become hard to implement this interface.
Would it be better to rather have the original `ExecutorService` as is, and then inject that executor service to the `DefaultProcessingPool`? The `ExecutorService` interface is richer and common. `QueryProcessingPool` can be used in cases where a `PrioritizedQueryRunnerCallable` submission is needed.
--
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.
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
[GitHub] [druid] jihoonson commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r658888746
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
Review comment:
```suggestion
* An implementation of {@link PrioritizedCallable} that also let caller get access to associated {@link QueryRunner}
```
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r659264943
##########
File path: processing/src/main/java/org/apache/druid/query/QueryProcessingPool.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.ListeningExecutorService;
+import org.apache.druid.guice.annotations.ExtensionPoint;
+
+/**
+ * This class implements the logic of how units of query execution run concurrently. It is used in {@link QueryRunnerFactory#mergeRunners(QueryProcessingPool, Iterable)}.
+ * In a most straightforward implementation, each unit will be submitted to an {@link PrioritizedExecutorService}. Extensions,
+ * however, can implement their own logic for picking which unit to pick first for execution.
+ * <p>
+ * This interface is convertible to a regular {@link java.util.concurrent.ExecutorService} as well. It has a separate
+ * method to submit query execution tasks so that implementations can differentiate those tasks from any regular async
+ * tasks. One example is {@link org.apache.druid.query.groupby.strategy.GroupByStrategyV2#mergeRunners(QueryProcessingPool, Iterable)}
+ * where different kind of tasks are submitted to same processing pool.
+ * <p>
+ * Query execution task also includes a reference to {@link QueryRunner} so that any state required to decide the priority
+ * of a unit can be carried forward with the corresponding {@link QueryRunner}.
+ */
+@ExtensionPoint
+public interface QueryProcessingPool
+{
+ /**
+ * Submits the query execution unit task for asynchronous execution.
+ *
+ * @param task - Task to be submitted.
+ * @param <T> - Task result type
+ * @param <V> - Query runner sequence type
+ * @return - Future object for tracking the task completion.
+ */
+ <T, V> ListenableFuture<T> submit(PrioritizedQueryRunnerCallable<T, V> task);
+
+ /**
+ * @return - Returns this pool as an executor service that can be used for other asynchronous operations.
+ */
+ ListeningExecutorService asExecutorService();
Review comment:
@rohangarg @jon-wei @jihoonson What do you think?
--
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
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #11382: Replace Processing ExecutorService with QueryProcessingPool
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #11382:
URL: https://github.com/apache/druid/pull/11382#discussion_r659264643
##########
File path: processing/src/main/java/org/apache/druid/query/PrioritizedQueryRunnerCallable.java
##########
@@ -17,20 +17,15 @@
* under the License.
*/
-package org.apache.druid.guice.annotations;
-
-import com.google.inject.BindingAnnotation;
-
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
+package org.apache.druid.query;
/**
+ * An implementation of {@link PrioritizedCallable} that also let's caller get access to associated {@link QueryRunner}
+ * It is used in implementations of {@link QueryRunnerFactory}
+ * @param <T>
+ * @param <V>
*/
-@BindingAnnotation
-@Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD})
-@Retention(RetentionPolicy.RUNTIME)
-public @interface Processing
+public interface PrioritizedQueryRunnerCallable<T, V> extends PrioritizedCallable<T>
{
+ QueryRunner<V> getRunner();
Review comment:
sure thing.
--
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