mcvsubbu commented on PR #13661: URL: https://github.com/apache/pinot/pull/13661#issuecomment-2241205157
> > The BaseBrokerStarter class returns the delegate class from the getBrokerRequestHandler() handler. So, this class (and the handleRequest() methods within this class) are meant to be external interface to a broker. So, i am not sure what you mean when you say that we may not use delegate all the time. Can you explain a bit more? > > You are correct. Currently we always create a delegate even without the multi-stage request handler, so it should be good to set it in the delegate. > > > Secondly, yes, installations may have some code before (and after return from) handleRequest(). Dont you think that the callers of handleRequest() should measure their own overhead if they are doing pre and post processing and add it on to the value returned by handleRequest() call for query time? It does not seem to be correct that the metric emitted includes only the pre-processing time but not the post-processing time. Agree? > > Basically we want to include the query parsing time during the request handling. In `PinotClientRequest` class, we parse the query first to detect the sql type of the query, then pass the query to the request handler. IMO ideally we should set the `requestArrivalTimeMs` as the time when broker sees the query, which is outside of the broker request handler. We can definitely check it again within the request handler to be more robust, and I'd suggest adding some comments because that code path won't be hit in the open source code. I suggest setting it (without any if check) inside the handleRequest() call, like it was before. If this is ok with you, I will remove the `if` around the set. The contract with the broker for external entities should be that the handleRequest() call provides the service, and emits metric that includes the time taken for the service. Agreed? -- 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
