You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "KKcorps (via GitHub)" <gi...@apache.org> on 2023/03/01 20:16:17 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #10364: Add operator level stats to response when tracing is enabled
KKcorps opened a new pull request, #10364:
URL: https://github.com/apache/pinot/pull/10364
Instructions:
1. The PR has to be tagged with at least one of the following labels (*):
1. `feature`
2. `bugfix`
3. `performance`
4. `ui`
5. `backward-incompat`
6. `release-notes` (**)
2. Remove these instructions before publishing the PR.
(*) Other labels to consider:
- `testing`
- `dependencies`
- `docker`
- `kubernetes`
- `observability`
- `security`
- `code-style`
- `extension-point`
- `refactor`
- `cleanup`
(**) Use `release-notes` label for scenarios like:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
--
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
[GitHub] [pinot] KKcorps commented on pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on PR #10364:
URL: https://github.com/apache/pinot/pull/10364#issuecomment-1455394727
@walterddr can you take another look?
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10364:
URL: https://github.com/apache/pinot/pull/10364#discussion_r1124099328
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -117,6 +118,13 @@ public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstan
String operatorId = metadata.get(DataTable.MetadataKey.OPERATOR_ID.getName());
if (operatorId != null) {
_operatorIds.add(operatorId);
+ if (_enableTrace) {
+ Map<String, String> metadataWithoutOperatorId = new HashMap<>(metadata);
+ metadataWithoutOperatorId.remove(DataTable.MetadataKey.OPERATOR_ID);
Review Comment:
Only reason is because the response looks unclean. The key is already operatorID and inside the map we put operatorId again.
--
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
[GitHub] [pinot] KKcorps merged pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps merged PR #10364:
URL: https://github.com/apache/pinot/pull/10364
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10364:
URL: https://github.com/apache/pinot/pull/10364#discussion_r1124099328
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -117,6 +118,13 @@ public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstan
String operatorId = metadata.get(DataTable.MetadataKey.OPERATOR_ID.getName());
if (operatorId != null) {
_operatorIds.add(operatorId);
+ if (_enableTrace) {
+ Map<String, String> metadataWithoutOperatorId = new HashMap<>(metadata);
+ metadataWithoutOperatorId.remove(DataTable.MetadataKey.OPERATOR_ID);
Review Comment:
Only reason is because the response looks unclean. The key is already operatorID and inside the operatorStatsMap we put operatorId again.
--
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
[GitHub] [pinot] KKcorps commented on a diff in pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #10364:
URL: https://github.com/apache/pinot/pull/10364#discussion_r1124110122
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -89,7 +90,7 @@ public void aggregate(ServerRoutingInstance routingInstance, DataTable dataTable
public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstance, Map<String, String> metadata,
Map<Integer, String> exceptions) {
// Reduce on trace info.
- if (_enableTrace) {
+ if (_enableTrace && metadata.containsKey(DataTable.MetadataKey.TRACE_INFO.getName())) {
Review Comment:
NullPointerException without the second check. Reason being the map doesn't contain traceInfo even when tracing i enabled for V2Engine.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -89,7 +90,7 @@ public void aggregate(ServerRoutingInstance routingInstance, DataTable dataTable
public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstance, Map<String, String> metadata,
Map<Integer, String> exceptions) {
// Reduce on trace info.
- if (_enableTrace) {
+ if (_enableTrace && metadata.containsKey(DataTable.MetadataKey.TRACE_INFO.getName())) {
Review Comment:
NullPointerException without the second check. Reason being the map doesn't contain traceInfo even when tracing is enabled for V2Engine.
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10364:
URL: https://github.com/apache/pinot/pull/10364#discussion_r1125910207
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -89,7 +90,7 @@ public void aggregate(ServerRoutingInstance routingInstance, DataTable dataTable
public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstance, Map<String, String> metadata,
Map<Integer, String> exceptions) {
// Reduce on trace info.
- if (_enableTrace) {
+ if (_enableTrace && metadata.containsKey(DataTable.MetadataKey.TRACE_INFO.getName())) {
Review Comment:
i see. we should clean this up later. it is not easy to track the code when we have multiple way to determine the same config.
either we
1. dont set _enableTrace as member variable and always deduce from metadata
2. make _enableTrace correct without needing to look at metadata
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10364:
URL: https://github.com/apache/pinot/pull/10364#issuecomment-1450880927
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10364](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ddea042) into [master](https://codecov.io/gh/apache/pinot/commit/c5dd6df27c384b523c77adcb66956bf0e5c37251?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c5dd6df) will **increase** coverage by `9.47%`.
> The diff coverage is `47.36%`.
```diff
@@ Coverage Diff @@
## master #10364 +/- ##
============================================
+ Coverage 60.86% 70.34% +9.47%
- Complexity 5007 5220 +213
============================================
Files 2020 2035 +15
Lines 109752 110238 +486
Branches 16693 16750 +57
============================================
+ Hits 66804 77545 +10741
+ Misses 37925 27262 -10663
- Partials 5023 5431 +408
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.50% <47.36%> (-0.01%)` | :arrow_down: |
| integration2 | `24.57% <5.26%> (?)` | |
| unittests1 | `67.75% <46.15%> (+0.04%)` | :arrow_up: |
| unittests2 | `13.76% <0.00%> (?)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...common/response/broker/BrokerResponseNativeV2.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvYnJva2VyL0Jyb2tlclJlc3BvbnNlTmF0aXZlVjIuamF2YQ==) | `33.33% <0.00%> (ø)` | |
| [...t/controller/api/resources/PinotQueryResource.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90UXVlcnlSZXNvdXJjZS5qYXZh) | `53.84% <0.00%> (+42.65%)` | :arrow_up: |
| [...ot/core/query/reduce/ExecutionStatsAggregator.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRXhlY3V0aW9uU3RhdHNBZ2dyZWdhdG9yLmphdmE=) | `96.69% <33.33%> (-0.87%)` | :arrow_down: |
| [...requesthandler/MultiStageBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `67.70% <75.00%> (-0.04%)` | :arrow_down: |
| [...ot/common/response/broker/BrokerResponseStats.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVzcG9uc2UvYnJva2VyL0Jyb2tlclJlc3BvbnNlU3RhdHMuamF2YQ==) | `95.65% <100.00%> (ø)` | |
| [...g/apache/pinot/server/api/resources/ErrorInfo.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvYXBpL3Jlc291cmNlcy9FcnJvckluZm8uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t/server/api/resources/DefaultExceptionMapper.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvYXBpL3Jlc291cmNlcy9EZWZhdWx0RXhjZXB0aW9uTWFwcGVyLmphdmE=) | `0.00% <0.00%> (-75.00%)` | :arrow_down: |
| [.../pinot/server/api/resources/TableSizeResource.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvYXBpL3Jlc291cmNlcy9UYWJsZVNpemVSZXNvdXJjZS5qYXZh) | `80.00% <0.00%> (-8.00%)` | :arrow_down: |
| [...ot/query/runtime/executor/RoundRobinScheduler.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9leGVjdXRvci9Sb3VuZFJvYmluU2NoZWR1bGVyLmphdmE=) | `86.90% <0.00%> (-5.96%)` | :arrow_down: |
| [.../predicate/NotEqualsPredicateEvaluatorFactory.java](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL05vdEVxdWFsc1ByZWRpY2F0ZUV2YWx1YXRvckZhY3RvcnkuamF2YQ==) | `68.75% <0.00%> (-5.36%)` | :arrow_down: |
| ... and [431 more](https://codecov.io/gh/apache/pinot/pull/10364?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10364: Add operator level stats to response when tracing is enabled
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10364:
URL: https://github.com/apache/pinot/pull/10364#discussion_r1123992574
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -89,7 +90,7 @@ public void aggregate(ServerRoutingInstance routingInstance, DataTable dataTable
public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstance, Map<String, String> metadata,
Map<Integer, String> exceptions) {
// Reduce on trace info.
- if (_enableTrace) {
+ if (_enableTrace && metadata.containsKey(DataTable.MetadataKey.TRACE_INFO.getName())) {
Review Comment:
why do we need both checks?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -117,6 +118,13 @@ public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstan
String operatorId = metadata.get(DataTable.MetadataKey.OPERATOR_ID.getName());
if (operatorId != null) {
_operatorIds.add(operatorId);
+ if (_enableTrace) {
+ Map<String, String> metadataWithoutOperatorId = new HashMap<>(metadata);
+ metadataWithoutOperatorId.remove(DataTable.MetadataKey.OPERATOR_ID);
+ _operatorStats.put(operatorId, metadataWithoutOperatorId);
+ } else {
+ _operatorStats.put(operatorId, new HashMap<>());
Review Comment:
with this `_operatorIds` can be removed.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/ExecutionStatsAggregator.java:
##########
@@ -117,6 +118,13 @@ public synchronized void aggregate(@Nullable ServerRoutingInstance routingInstan
String operatorId = metadata.get(DataTable.MetadataKey.OPERATOR_ID.getName());
if (operatorId != null) {
_operatorIds.add(operatorId);
+ if (_enableTrace) {
+ Map<String, String> metadataWithoutOperatorId = new HashMap<>(metadata);
+ metadataWithoutOperatorId.remove(DataTable.MetadataKey.OPERATOR_ID);
Review Comment:
no need to remove the key itself IMO. it is not that much of an overhead compared to doing a map copy.
--
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