yashmayya commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2644131695
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -233,6 +235,11 @@ public class BrokerMeter implements AbstractMetrics.Meter {
*/
public static final BrokerMeter WINDOW_COUNT = create("WINDOW_COUNT",
"queries", true);
+ public static final BrokerMeter MSE_STAGES_STARTED =
create("MSE_STAGE_STARTED", "stages", false);
+ public static final BrokerMeter MSE_STAGES_COMPLETED =
create("MSE_STAGE_COMPLETED", "stages", false);
+ public static final BrokerMeter MSE_OPCHAINS_STARTED =
create("MSE_OPCHAIN_STARTED", "opchains", false);
+ public static final BrokerMeter MSE_OPCHAINS_COMPLETED =
create("MSE_OPCHAIN_COMPLETED", "opchains", false);
Review Comment:
Shouldn't the global parameter be true for all of these?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -581,10 +587,19 @@ private BrokerResponse
query(QueryEnvironment.CompiledQuery query, long requestI
return new BrokerResponseNative(QueryErrorCode.EXECUTION_TIMEOUT);
}
+ int stageCount = dispatchableSubPlan.getQueryStageMap().size();
+ int opChainCount = dispatchableSubPlan.getQueryStageMap().values().stream()
+ .mapToInt(stage -> stage.getServerInstances().size())
Review Comment:
Shouldn't we use the worker info instead of server info since we could
potentially have multiple workers per server?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -136,6 +137,11 @@ public class MultiStageBrokerRequestHandler extends
BaseBrokerRequestHandler {
protected final long _extraPassiveTimeoutMs;
protected final boolean _enableQueryFingerprinting;
+ protected final PinotMeter _stagesStartedMeter =
BrokerMeter.MSE_STAGES_STARTED.getGlobalMeter();
+ protected final PinotMeter _stagesFinishedMeter =
BrokerMeter.MSE_STAGES_COMPLETED.getGlobalMeter();
+ protected final PinotMeter _opchainsStartedMeter =
BrokerMeter.MSE_OPCHAINS_STARTED.getGlobalMeter();
+ protected final PinotMeter _opchainsCompletedMeter =
BrokerMeter.MSE_OPCHAINS_COMPLETED.getGlobalMeter();
Review Comment:
The `BaseBrokerRequestHandler` class already has an instance of
`BrokerMetrics`. We should use that here instead for better testability instead
of the global singleton IMO.
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##########
@@ -233,6 +235,11 @@ public class BrokerMeter implements AbstractMetrics.Meter {
*/
public static final BrokerMeter WINDOW_COUNT = create("WINDOW_COUNT",
"queries", true);
+ public static final BrokerMeter MSE_STAGES_STARTED =
create("MSE_STAGE_STARTED", "stages", false);
+ public static final BrokerMeter MSE_STAGES_COMPLETED =
create("MSE_STAGE_COMPLETED", "stages", false);
+ public static final BrokerMeter MSE_OPCHAINS_STARTED =
create("MSE_OPCHAIN_STARTED", "opchains", false);
+ public static final BrokerMeter MSE_OPCHAINS_COMPLETED =
create("MSE_OPCHAIN_COMPLETED", "opchains", false);
Review Comment:
```suggestion
public static final BrokerMeter MSE_STAGES_STARTED =
create("MSE_STAGES_STARTED", "stages", false);
public static final BrokerMeter MSE_STAGES_COMPLETED =
create("MSE_STAGES_COMPLETED", "stages", false);
public static final BrokerMeter MSE_OPCHAINS_STARTED =
create("MSE_OPCHAINS_STARTED", "opchains", false);
public static final BrokerMeter MSE_OPCHAINS_COMPLETED =
create("MSE_OPCHAINS_COMPLETED", "opchains", false);
```
nit: for consistency
--
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]
