walterddr commented on code in PR #10337: URL: https://github.com/apache/pinot/pull/10337#discussion_r1119132865
########## pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseStats.java: ########## @@ -0,0 +1,106 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.response.broker; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonPropertyOrder; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.apache.pinot.spi.utils.JsonUtils; + + +@JsonPropertyOrder({"exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "numServersQueried", + "numServersResponded", "numSegmentsQueried", "numSegmentsProcessed", "numSegmentsMatched", + "numConsumingSegmentsQueried", "numConsumingSegmentsProcessed", "numConsumingSegmentsMatched", + "numDocsScanned", "numEntriesScannedInFilter", "numEntriesScannedPostFilter", "numGroupsLimitReached", + "totalDocs", "timeUsedMs", "offlineThreadCpuTimeNs", "realtimeThreadCpuTimeNs", + "offlineSystemActivitiesCpuTimeNs", "realtimeSystemActivitiesCpuTimeNs", "offlineResponseSerializationCpuTimeNs", + "realtimeResponseSerializationCpuTimeNs", "offlineTotalCpuTimeNs", "realtimeTotalCpuTimeNs", + "traceInfo", "operatorIds", "tableNames"}) +@JsonInclude(JsonInclude.Include.NON_DEFAULT) +public class BrokerResponseStats extends BrokerResponseNative { Review Comment: add a TODO: let's think of a way to 1. cleanly decouple the v1 and v2 response native format. - decouple the execution stats aggregator logic and make it into a util that can aggregate 2 values with the same metadataKey (not tied with member variable based implementation) 2. v2 response stats format should not store information that's not relevant - for example intermediate stage stats should not kept any metadatakey related to leaf stages - suggest use a Map<MetadataKey, Object> 3. v2 response stats should be configurable to any level (global/stage/operator) -they should reuse the same BrokerResponseStats (or just QueryStats) class and can be configured with Member variable field SubStats --> global will have stage subStats; and stage will have operator subStats Follow up on separate PR. this one looks good for now. ########## pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java: ########## @@ -82,6 +93,32 @@ public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstan _traceInfo.put(routingInstance.getShortName(), metadata.get(DataTable.MetadataKey.TRACE_INFO.getName())); } + String tableNamesStr = metadata.get(DataTable.MetadataKey.TABLE.getName()); + String tableName = null; + + if (tableNamesStr != null) { + List<String> tableNames = Arrays.stream(tableNamesStr.split("::")).collect(Collectors.toList()); + _tableNames.addAll(tableNames); + + //TODO: Decide a strategy to split stageLevel stats across tables for brokerMetrics + // assigning everything to the first table only for now + tableName = tableNames.get(0); + } Review Comment: great catch and let's follow up in separate PR. ########## pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseStats.java: ########## @@ -0,0 +1,106 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.response.broker; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonPropertyOrder; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import org.apache.pinot.spi.utils.JsonUtils; + + +@JsonPropertyOrder({"exceptions", "numBlocks", "numRows", "stageExecutionTimeMs", "numServersQueried", Review Comment: TODO: follow up to not encode the metadata key as string, (to avoid ser/de overhead) ########## pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTable.java: ########## @@ -102,16 +102,14 @@ enum MetadataValueType { */ enum MetadataKey { UNKNOWN(0, "unknown", MetadataValueType.STRING), - TABLE(1, "table", MetadataValueType.STRING), // NOTE: this key is only used in PrioritySchedulerTest + TABLE(1, "table", MetadataValueType.STRING), Review Comment: chatted offline. we are going to use MetadataKey going forward for compact metadata index perspective (so all the stats map on stage/operator/global level they all will be indexed/keyed by the integer) -- 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
