You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2025/12/23 12:06:43 UTC
[PR] Add new mse metrics [pinot]
gortiz opened a new pull request, #17419:
URL: https://github.com/apache/pinot/pull/17419
This PR adds new metrics for MSE:
On broker:
- Total stages started/finished
- Total opchains started/finished
On server:
- Total opchain started/executed
- CPU time for each task
- Memory allocation for each task
- Emitted docs
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2643480537
##########
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()
Review Comment:
The dispatchableSubPlan.getQueryStageMap() is called twice. Consider storing the result in a variable to avoid redundant calls.
```suggestion
Map<Integer, DispatchablePlanFragment> queryStageMap = dispatchableSubPlan.getQueryStageMap();
int stageCount = queryStageMap.size();
int opChainCount = queryStageMap.values().stream()
```
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +169,25 @@ public Object getAny(K key) {
}
}
+ /**
+ * Returns the value associated with the key name.
+ *
+ * In general, it is better to use the type-specific getters with the enum key directly, but sometimes it is
+ * impossible or requires complex to read code (like complex unsafe casts).
+ *
+ * @param keyName The name of the key.
+ * @param defaultValue The default value to return if the key is not found.
+ * @throws ClassCastException if the value cannot be cast to the same static type as the default value.
+ */
+ public <E> E getUnsafe(String keyName, E defaultValue)
+ throws ClassCastException {
+ K key = getKey(keyName);
+ if (key == null) {
+ return defaultValue;
+ }
+ return (E) getAny(key);
Review Comment:
The getUnsafe() method should document the behavior when the key exists but has a null value, as getAny() can return null. Currently, it's unclear whether this would return null or defaultValue.
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +169,25 @@ public Object getAny(K key) {
}
}
+ /**
+ * Returns the value associated with the key name.
+ *
+ * In general, it is better to use the type-specific getters with the enum key directly, but sometimes it is
+ * impossible or requires complex to read code (like complex unsafe casts).
Review Comment:
The phrase 'requires complex to read code' should be 'requires complex-to-read code' or 'requires code that is complex to read'.
```suggestion
* impossible or requires complex-to-read code (like complex unsafe casts).
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #17419:
URL: https://github.com/apache/pinot/pull/17419
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2653501379
##########
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 attribute in the parent class is actually final and initialized to the value used here (the one returned by `BrokerMetrics.get()`).
In general, the idea is to use `BrokerMetrics.get()` whenever possible. We should also keep the meters as attributes, as I'm doing here, to speed up calls (since we don't need to look up the meter each time). In `BaseBrokerRequestHandler` we use the old pattern, where we have an attribute for `BrokerMetrics` but look for the metric each time we need to modify 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #17419:
URL: https://github.com/apache/pinot/pull/17419#issuecomment-3719168422
I didn't see that some tests were failing because they mocked multi-stage operators, and the copyStats method was returning null. Fixed
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2653511289
##########
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:
Fixed here and in server meters
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17419:
URL: https://github.com/apache/pinot/pull/17419#issuecomment-3686579702
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17419?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `30.61224%` with `34 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 33.91%. Comparing base ([`6d4df46`](https://app.codecov.io/gh/apache/pinot/commit/6d4df46e7888b81d7237ee7fdf2a067a22d08ba9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f61a619`](https://app.codecov.io/gh/apache/pinot/commit/f61a6192e1d58f1c89ac112db8f75040688f49d6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
:white_check_mark: All tests successful. No failed tests found.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17419?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...uery/runtime/executor/OpChainSchedulerService.java](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fexecutor%2FOpChainSchedulerService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9PcENoYWluU2NoZWR1bGVyU2VydmljZS5qYXZh) | 0.00% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...requesthandler/MultiStageBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&filepath=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Frequesthandler%2FMultiStageBrokerRequestHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | 33.33% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ava/org/apache/pinot/common/datatable/StatMap.java](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fdatatable%2FStatMap.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZGF0YXRhYmxlL1N0YXRNYXAuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/common/metrics/ServerMeter.java](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FServerMeter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJNZXRlci5qYXZh) | 85.71% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17419?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (6d4df46) and HEAD (f61a619). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (6d4df46) | HEAD (f61a619) |
>|------|------|------|
>|unittests1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17419 +/- ##
=============================================
- Coverage 55.65% 33.91% -21.75%
- Complexity 702 765 +63
=============================================
Files 2463 3153 +690
Lines 139016 187954 +48938
Branches 22167 28763 +6596
=============================================
- Hits 77376 63743 -13633
- Misses 55119 118928 +63809
+ Partials 6521 5283 -1238
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17419/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17419/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.91% <30.61%> (-21.75%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17419/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.91% <30.61%> (-21.75%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17419/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.91% <30.61%> (-21.75%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17419/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17419/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.91% <30.61%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17419?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2653510637
##########
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:
You are right, but I didn't find that and it looks like right now we don't schedule more than one worker per server.
Should I use `stage.getWorkerMetadataList` instead?
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2670489696
##########
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:
> You are right, but I didn't find that and it looks like right now we don't schedule more than one worker per server.
It's possible when intermediate stage parallelism is configured via query option. There's some other scenarios too where more than one worker can be assigned to each server.
> Should I use stage.getWorkerMetadataList instead
Yup, that's the right API to use here IMO.
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add new mse metrics [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17419:
URL: https://github.com/apache/pinot/pull/17419#discussion_r2642984195
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +166,25 @@ public Object getAny(K key) {
}
}
+ /// Returns the value associated with the key name.
+ ///
+ /// In general, it is better to use the type-specific getters with the enum key directly, but sometimes it is
+ /// impossible or requires complex to read code (like complex unsafe casts).
+ ///
+ /// @param keyName The name of the key.
+ /// @param defaultValue The default value to return if the key is not found.
+ /// @throws ClassCastException if the value cannot be cast to the same static type as the default value.
+ public <E> E getUnsafe(String keyName, E defaultValue)
Review Comment:
The method name `getUnsafe` is ambiguous. It's unclear whether 'unsafe' refers to the lack of type safety, potential for exceptions, or unchecked access. Consider `getByName` or `getByKeyName` to better convey that it performs string-based lookup.
```suggestion
public <E> E getByKeyName(String keyName, E defaultValue)
```
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +166,25 @@ public Object getAny(K key) {
}
}
+ /// Returns the value associated with the key name.
+ ///
+ /// In general, it is better to use the type-specific getters with the enum key directly, but sometimes it is
+ /// impossible or requires complex to read code (like complex unsafe casts).
+ ///
+ /// @param keyName The name of the key.
+ /// @param defaultValue The default value to return if the key is not found.
+ /// @throws ClassCastException if the value cannot be cast to the same static type as the default value.
+ public <E> E getUnsafe(String keyName, E defaultValue)
+ throws ClassCastException {
+ K[] keys = keys();
+ for (K key : keys) {
+ if (key.name().equals(keyName)) {
+ return (E) getAny(key);
+ }
+ }
+ return defaultValue;
+ }
Review Comment:
The linear search through all keys on every call is inefficient. For better performance, consider caching a map of key names to enum keys during initialization, especially since this method appears to be called frequently (once per opchain completion).
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/executor/OpChainSchedulerService.java:
##########
@@ -266,4 +273,24 @@ private int countOperators(MultiStageOperator root) {
private ReadWriteLock getQueryLock(long requestId) {
return _queryLocks[(int) (requestId & QUERY_LOCK_MASK)];
}
+
+ private static class Metrics {
+ private final PinotMeter _startedOpchains = ServerMeter.MSE_OPCHAINS_STARTED.getGlobalMeter();
+ private final PinotMeter _competedOpchains = ServerMeter.MSE_OPCHAINS_COMPLETED.getGlobalMeter();
+ private final PinotMeter _emittedRows = ServerMeter.MSE_EMITTED_ROWS.getGlobalMeter();
+ private final PinotMeter _cpuExecutionTimeMs = ServerMeter.MSE_CPU_EXECUTION_TIME_MS.getGlobalMeter();
+ private final PinotMeter _memoryAllocatedBytes = ServerMeter.MSE_MEMORY_ALLOCATED_BYTES.getGlobalMeter();
+
+ public void onOpChainStarted() {
+ _startedOpchains.mark();
+ }
+
+ public void onOpChainFinished(MultiStageOperator rootOperator) {
+ _competedOpchains.mark();
+ StatMap<?> operatorStats = rootOperator.copyStatMaps();
+ _emittedRows.mark(operatorStats.getUnsafe("EMITTED_ROWS", 0L));
+ _cpuExecutionTimeMs.mark(operatorStats.getUnsafe("EXECUTION_TIME_MS", 0L));
+ _memoryAllocatedBytes.mark(operatorStats.getUnsafe("ALLOCATED_MEMORY_BYTES", 0L));
Review Comment:
Hard-coded string keys like 'EMITTED_ROWS', 'EXECUTION_TIME_MS', and 'ALLOCATED_MEMORY_BYTES' are error-prone and difficult to maintain. These should be defined as constants or enum values to ensure consistency and enable compile-time checking.
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/StatMap.java:
##########
@@ -166,6 +166,25 @@ public Object getAny(K key) {
}
}
+ /// Returns the value associated with the key name.
+ ///
+ /// In general, it is better to use the type-specific getters with the enum key directly, but sometimes it is
+ /// impossible or requires complex to read code (like complex unsafe casts).
+ ///
+ /// @param keyName The name of the key.
+ /// @param defaultValue The default value to return if the key is not found.
+ /// @throws ClassCastException if the value cannot be cast to the same static type as the default value.
Review Comment:
The method documentation uses `///` style comments but should consistently use `/** ... */` for public API methods according to Java conventions. The `@throws` clause is missing from the documentation.
```suggestion
/**
* Returns the value associated with the key name.
* <p>
* In general, it is better to use the type-specific getters with the enum key directly, but sometimes it is
* impossible or requires complex to read code (like complex unsafe casts).
*
* @param keyName The name of the key.
* @param defaultValue The default value to return if the key is not found.
* @param <E> The static type of the value to return.
* @return The value associated with the key name, or {@code defaultValue} if the key is not found.
* @throws ClassCastException if the value cannot be cast to the same static type as the default value.
*/
```
--
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org