[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #6027: Support streaming query in QueryExecutor

[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