gortiz commented on code in PR #13733:
URL: https://github.com/apache/pinot/pull/13733#discussion_r1754550758
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/DispatchClient.java:
##########
@@ -50,12 +51,17 @@ public ManagedChannel getChannel() {
}
public void submit(Worker.QueryRequest request, QueryServerInstance
virtualServer, Deadline deadline,
- Consumer<AsyncQueryDispatchResponse> callback) {
- _dispatchStub.withDeadline(deadline).submit(request, new
DispatchObserver(virtualServer, callback));
+ Consumer<AsyncResponse<Worker.QueryResponse>> callback) {
+ _dispatchStub.withDeadline(deadline).submit(request, new
LastValueDispatchObserver<>(virtualServer, callback));
}
public void cancel(long requestId) {
Worker.CancelRequest cancelRequest =
Worker.CancelRequest.newBuilder().setRequestId(requestId).build();
_dispatchStub.cancel(cancelRequest, NO_OP_CANCEL_STREAM_OBSERVER);
}
+
+ public void explain(Worker.QueryRequest request, QueryServerInstance
virtualServer, Deadline deadline,
+ Consumer<AsyncResponse<List<Worker.ExplainResponse>>> callback) {
+ _dispatchStub.withDeadline(deadline).explain(request, new
AllValuesDispatchObserver<>(virtualServer, callback));
Review Comment:
I don't know why we only keep the last message in the submit case, but you
can see the older implementation is using
[DispatchObserver](https://github.com/apache/pinot/blob/master/pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/DispatchObserver.java)
and verify that it is keeping only the latest `QueryResponse` received in
`onNext`, which is what `LastValueDispatcherObserver` does.
TBH the old behavior is correct. You can see that
`org.apache.pinot.query.service.server.QueryServer.submit` does only call
`onNext` once, so it is ok to keep the last value. What I don't get is why the
proto file is defined as:
```
rpc Submit(ServerRequest) returns (stream ServerResponse);
```
instead of
```
rpc Submit(ServerRequest) returns (ServerResponse);
```
Anyway, if we change the server implementation we will need to use the new
`AllValuesDispatcherObserver` and change the code a bit to work with a list
instead of a single element.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
