You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/09/17 06:37:33 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #6027: Support streaming query in QueryExecutor
Jackie-Jiang opened a new pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027
## Description
Add `StreamObserver` into `QueryExecutor.processQuery()` method so that the gRPC streaming query server can reuse the `QueryExecutor`.
Remove `GrpcQueryExecutor` which is almost identical to `ServerQueryExecutorV1Impl`.
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027#discussion_r490610006
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/executor/QueryExecutor.java
##########
@@ -54,6 +55,16 @@ void init(PinotConfiguration config, InstanceDataManager instanceDataManager, Se
/**
* Processes the query with the given executor service.
+ * <ul>
+ * <li>
+ * For streaming request, the returned DataTable contains only the metadata. The response is streamed back via the
+ * observer.
+ * </li>
+ * <li>
+ * For non-streaming request, the returned DataTable contains both data and metadata.
+ * </li>
+ * </ul>
*/
- DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService);
+ DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService,
Review comment:
I agree. We should consider having a different implementation.
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027#discussion_r490610341
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
##########
@@ -94,7 +96,8 @@ public synchronized void shutDown() {
}
@Override
- public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService) {
+ public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService,
+ @Nullable StreamObserver<Server.ServerResponse> responseObserver) {
Review comment:
I think it is possible to have a separate implementation. Gradually over time, this might need more changes and we might find ourselves polluting this method for streaming v/s non-streaming if branches.
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027#issuecomment-695141063
Discussed with @siddharthteotia and @fx19880617 offline and we all agreed that `QueryExecutor` logic for streaming and non-streaming query will remain very similar, and the actual streaming handling is handled on the `Operator` level.
Instead of changing the current `processQuery()` method, decide to add another method for the streaming query so that in the future we can easily split them if necessary.
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027#issuecomment-694558309
@fx19880617 @siddharthteotia The current code (without this PR) is the approach of using separate implementation for streaming query, and turns out the code is almost identical (check `GrpcQueryExecutor`). Whenever we made changes to the query executor, we have to change 2 places and keep them in sync, which is the reason why I want to merge them into one.
I tried to extract out the common part for these 2 implementations, then find that the cleanest way is just keeping one implementation. An alternative way is to make the `processQuery()` a helper method, and then make 2 implementations to call the same method. IMO, that's not as readable.
The new `processQuery()` can handle both streaming and non-streaming query, which IMO is more desired as we want to keep both mode in the future.
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027#discussion_r490610341
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
##########
@@ -94,7 +96,8 @@ public synchronized void shutDown() {
}
@Override
- public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService) {
+ public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService,
+ @Nullable StreamObserver<Server.ServerResponse> responseObserver) {
Review comment:
I think it is possible to have a separate implementation. Gradually over time, this might need more changes and we will find ourselves polluting this method for streaming v/s non-streaming if branches.
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] siddharthteotia commented on a change in pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027#discussion_r490610341
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
##########
@@ -94,7 +96,8 @@ public synchronized void shutDown() {
}
@Override
- public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService) {
+ public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService,
+ @Nullable StreamObserver<Server.ServerResponse> responseObserver) {
Review comment:
I think it is possible to have a separate implementation. Gradually over time, this might need more changes we wiull find ourselves polluting this method for streaming v/s non-streaming if branches.
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #6027: Support streaming query in QueryExecutor
Posted by GitBox <gi...@apache.org>.
fx19880617 commented on a change in pull request #6027:
URL: https://github.com/apache/incubator-pinot/pull/6027#discussion_r490604509
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/executor/QueryExecutor.java
##########
@@ -54,6 +55,16 @@ void init(PinotConfiguration config, InstanceDataManager instanceDataManager, Se
/**
* Processes the query with the given executor service.
+ * <ul>
+ * <li>
+ * For streaming request, the returned DataTable contains only the metadata. The response is streamed back via the
+ * observer.
+ * </li>
+ * <li>
+ * For non-streaming request, the returned DataTable contains both data and metadata.
+ * </li>
+ * </ul>
*/
- DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService);
+ DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService executorService,
Review comment:
Shall we have a different QueryExecutor implementation for streaming request rather than change this method in the interface?
----------------------------------------------------------------
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@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org