Jackie-Jiang commented on code in PR #13177:
URL: https://github.com/apache/pinot/pull/13177#discussion_r1607298327
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcResultsBlockStreamer.java:
##########
@@ -44,6 +48,8 @@ public void send(BaseResultsBlock block)
Collection<Object[]> rows = block.getRows();
Preconditions.checkState(dataSchema != null && rows != null, "Malformed
data block");
DataTable dataTable = block.getDataTable();
- _streamObserver.onNext(StreamingResponseUtils.getDataResponse(dataTable));
+ Server.ServerResponse response =
StreamingResponseUtils.getDataResponse(dataTable);
+ _streamObserver.onNext(response);
+ _serverMetrics.addMeteredGlobalValue(ServerMeter.GRPC_BYTES_SENT,
response.getSerializedSize());
Review Comment:
Good catch on this bug. Can you also add this fix into the PR description?
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -53,6 +53,8 @@ public enum ServerTimer implements AbstractMetrics.Timer {
"Total time taken to preload a table partition of an upsert table with
upsert snapshot"),
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or
deletedKeysTTL"),
+ GRPC_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a
successful query over gRPC"),
+ GRPC_FAILED_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time
of a failing query over gRPC"),
Review Comment:
Failed query execution time might not be very useful
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerTimer.java:
##########
@@ -53,6 +53,8 @@ public enum ServerTimer implements AbstractMetrics.Timer {
"Total time taken to preload a table partition of an upsert table with
upsert snapshot"),
UPSERT_REMOVE_EXPIRED_PRIMARY_KEYS_TIME_MS("milliseconds", false,
"Total time taken to delete expired primary keys based on metadataTTL or
deletedKeysTTL"),
+ GRPC_QUERY_EXECUTION_MS("milliseconds", true, "Total execution time of a
successful query over gRPC"),
Review Comment:
Let's make this table level
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/grpc/GrpcQueryServer.java:
##########
@@ -72,19 +77,38 @@ public class GrpcQueryServer extends
PinotQueryServerGrpc.PinotQueryServerImplBa
private final AccessControl _accessControl;
private final ServerQueryLogger _queryLogger =
ServerQueryLogger.getInstance();
+ // Filter to keep track of gRPC connections.
+ private class GrpcQueryTransportFilter extends ServerTransportFilter {
+ @Override
+ public Attributes transportReady(Attributes transportAttrs) {
+ LOGGER.info("gRPC transportReady: REMOTE_ADDR {}",
Review Comment:
Will this potentially flood the log? Since we already have the metrics,
maybe no need to log it
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
