search

apache / pinot

Apache Pinot - A realtime distributed OLAP datastore
https://pinot.apache.org/
Apache License 2.0
5.39k stars 1.26k forks source link

returns requestId for each broker query request #10884

Open mingmxu opened 1 year ago

mingmxu commented 1 year ago

what's the problem?

Return a unique requestId always is important for tracing/debugging, to find related logging events for example. There're some limitation at the moment:

  1. it's not available to every queries in broker, onlyMultiStageBrokerRequestHandler generates one and included in BrokerResponseNativeV2;
  2. In java-client, org.apache.pinot.client.BrokerResponse is a reduced set of org.apache.pinot.common.response.BrokerResponse, requestId is lost with many other metadata fields;

Proposed changes

To address these limitations, I would like to suggest some changes as below:

Jackie-Jiang commented 1 year ago

Broker request id is generated within the BrokerRequestHandler, and I think we can keep it this way, but include it in the org.apache.pinot.common.response.BrokerResponse. Seems org.apache.pinot.client.BrokerResponse was introduced to avoid dependency on pinot-common, but the module already has dependency on pinot-common so we may remove it. We need to be cautious about backward compatibility though.

@mingmxu Do you want to help contribute this?

mingmxu commented 1 year ago

surely will submit a PR on it. The requestId can be generated and set in BrokerResponse at method org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler#handleRequest(), using MultiStageRequestIdGenerator as the default one for both v1/v2. I'll add a new requestId field in org.apache.pinot.client.BrokerResponse for now. Consolidating org.apache.pinot.client.BrokerResponse and org.apache.pinot.common.response.BrokerResponse would move to the next PR.