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/21 13:56:00 UTC
[GitHub] [pinot] KKcorps opened a new pull request, #10450: Bug fix: Start counting operator execution time from first NoOp block
KKcorps opened a new pull request, #10450:
URL: https://github.com/apache/pinot/pull/10450
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] walterddr commented on a diff in pull request #10450: Bug fix: Start counting operator execution time from first NoOp block
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10450:
URL: https://github.com/apache/pinot/pull/10450#discussion_r1144920736
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -64,8 +64,9 @@ public TransferableBlock nextBlock() {
try (InvocationScope ignored = Tracing.getTracer().createScope(getClass())) {
_operatorStats.startTimer();
TransferableBlock nextBlock = getNextBlock();
+ _operatorStats.endTimer(nextBlock);
Review Comment:
we dont have any tests for MultiStageOperator stats and trace. it is good to add a collection of these
--
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 #10450: Bug fix: Start counting operator execution time from first NoOp block
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10450:
URL: https://github.com/apache/pinot/pull/10450#discussion_r1143746039
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -64,8 +65,15 @@ public TransferableBlock nextBlock() {
try (InvocationScope ignored = Tracing.getTracer().createScope(getClass())) {
_operatorStats.startTimer();
TransferableBlock nextBlock = getNextBlock();
+
+ if (!_firstDataBlockEncountered && nextBlock.isNoOpBlock()) {
+ _operatorStats.resetStartTime();
+ } else {
+ _firstDataBlockEncountered = true;
+ _operatorStats.endTimer();
+ }
+
Review Comment:
Let's do it differently
```suggestion
_operatorStats.startTimer();
TransferableBlock nextBlock = getNextBlock();
_operatorStats.endTimer(nextBlock);
```
--
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 #10450: Bug fix: Start counting operator execution time from first NoOp block
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10450:
URL: https://github.com/apache/pinot/pull/10450#discussion_r1144918771
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##########
@@ -63,10 +65,21 @@ public void startTimer() {
}
}
- public void endTimer() {
+ public void endTimer(TransferableBlock block) {
if (_executeStopwatch.isRunning()) {
_executeStopwatch.stop();
}
+ if (!_processingStarted && block.isNoOpBlock()) {
+ _startTimeMs = -1;
+ _executeStopwatch.reset();
+ } else {
+ _processingStarted = true;
+ }
+ }
+
+ public void resetStartTime() {
+ _startTimeMs = -1;
+ _executeStopwatch.reset();
Review Comment:
remover as it is no longer used.
--
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 merged pull request #10450: Bug fix: Start counting operator execution time from first NoOp block
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10450:
URL: https://github.com/apache/pinot/pull/10450
--
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 #10450: Bug fix: Start counting operator execution time from first NoOp block
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10450:
URL: https://github.com/apache/pinot/pull/10450#issuecomment-1479540819
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10450?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 [#10450](https://codecov.io/gh/apache/pinot/pull/10450?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e652bd) into [master](https://codecov.io/gh/apache/pinot/commit/3687b2b732be22dc59f6ba08f90bce9e8b4ee45b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3687b2b) will **increase** coverage by `5.04%`.
> The diff coverage is `77.77%`.
```diff
@@ Coverage Diff @@
## master #10450 +/- ##
============================================
+ Coverage 63.23% 68.28% +5.04%
- Complexity 5905 6104 +199
============================================
Files 2036 2067 +31
Lines 110973 111933 +960
Branches 16892 17027 +135
============================================
+ Hits 70177 76435 +6258
+ Misses 35606 30034 -5572
- Partials 5190 5464 +274
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.37% <0.00%> (-0.12%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `67.91% <77.77%> (-0.05%)` | :arrow_down: |
| unittests2 | `13.92% <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/10450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/query/runtime/operator/OperatorStats.java](https://codecov.io/gh/apache/pinot/pull/10450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcGVyYXRvclN0YXRzLmphdmE=) | `87.75% <75.00%> (-4.93%)` | :arrow_down: |
| [...not/query/runtime/operator/MultiStageOperator.java](https://codecov.io/gh/apache/pinot/pull/10450?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aVN0YWdlT3BlcmF0b3IuamF2YQ==) | `79.54% <100.00%> (ø)` | |
... and [439 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10450/indirect-changes?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 #10450: Bug fix: Start counting operator execution time from first NoOp block
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10450:
URL: https://github.com/apache/pinot/pull/10450#discussion_r1143748627
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##########
@@ -69,6 +69,11 @@ public void endTimer() {
}
}
+ public void resetStartTime() {
+ _startTimeMs = -1;
+ _executeStopwatch.reset();
+ }
Review Comment:
instead change the endTimer method
```suggestion
public void endTimer(TransferableBlock block) {
if (_executeStopwatch.isRunning()) {
_executeStopwatch.stop();
}
if (block.isNoOpBlock() && processingStarted) {
_startTimeMs = -1;
_executeStopwatch.reset();
} else {
_processStarted = true;
}
}
```
--
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 #10450: Bug fix: Start counting operator execution time from first NoOp block
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10450:
URL: https://github.com/apache/pinot/pull/10450#discussion_r1143748627
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/OperatorStats.java:
##########
@@ -69,6 +69,11 @@ public void endTimer() {
}
}
+ public void resetStartTime() {
+ _startTimeMs = -1;
+ _executeStopwatch.reset();
+ }
Review Comment:
instead change the endTimer method
```suggestion
public void endTimer(TransferableBlock block) {
if (_executeStopwatch.isRunning()) {
_executeStopwatch.stop();
}
if (block.isNoOpBlock() && !processingStarted) {
_startTimeMs = -1;
_executeStopwatch.reset();
} else {
_processStarted = true;
}
}
```
--
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