You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2024/02/08 09:55:29 UTC
[PR] Add upsert-snapshot timer metric [pinot]
tibrewalpratik17 opened a new pull request, #12383:
URL: https://github.com/apache/pinot/pull/12383
label:
- `observability`
This patch adds metrics to report the time taken by the upsert-snapshot workflow.
Also improves logging by adding number of consuming segments. At present, log comes as:
```
Finished taking snapshot for 120 immutable segments with 7794903 primary keys (out of 121 total segments) in 2343ms
```
Adding number of consuming segments will give clarity if the 1 missed is consuming segment or not.
--
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 upsert-snapshot timer metric [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12383:
URL: https://github.com/apache/pinot/pull/12383#issuecomment-1933797833
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12383?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `3 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`041e040`)](https://app.codecov.io/gh/apache/pinot/commit/041e04078f5a94fca92c805a8db8fdf1f904a985?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.73% compared to head [(`7187616`)](https://app.codecov.io/gh/apache/pinot/pull/12383?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.55%.
> Report is 16 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...cal/upsert/BasePartitionUpsertMetadataManager.java](https://app.codecov.io/gh/apache/pinot/pull/12383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvQmFzZVBhcnRpdGlvblVwc2VydE1ldGFkYXRhTWFuYWdlci5qYXZh) | 57.14% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12383 +/- ##
============================================
- Coverage 61.73% 61.55% -0.18%
+ Complexity 1152 207 -945
============================================
Files 2424 2425 +1
Lines 132526 132722 +196
Branches 20483 20532 +49
============================================
- Hits 81810 81699 -111
- Misses 44720 45001 +281
- Partials 5996 6022 +26
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-61.67%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.55% <62.50%> (-0.06%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.55% <62.50%> (-0.16%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.60%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.55% <62.50%> (-0.18%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.55% <62.50%> (-0.18%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.67% <12.50%> (-0.25%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12383/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <50.00%> (+0.02%)` | :arrow_up: |
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/12383?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).
--
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 upsert-snapshot timer metric [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #12383:
URL: https://github.com/apache/pinot/pull/12383#discussion_r1491724125
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -660,8 +666,9 @@ protected void doTakeSnapshot() {
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId,
ServerGauge.UPSERT_PRIMARY_KEYS_IN_SNAPSHOT_COUNT, numPrimaryKeysInSnapshot);
_logger.info(
- "Finished taking snapshot for {} immutable segments with {} primary keys (out of {} total segments) in {}ms",
- numImmutableSegments, numPrimaryKeysInSnapshot, numTrackedSegments, System.currentTimeMillis() - startTimeMs);
+ "Finished taking snapshot for {} immutable segments with {} primary keys (out of {} total segments, "
+ + "{} consuming segments) in {} ms", numImmutableSegments, numPrimaryKeysInSnapshot, numTrackedSegments,
Review Comment:
nit: `{} are consuming segments` to be more clear
--
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 upsert-snapshot timer metric [pinot]
Posted by "deemoliu (via GitHub)" <gi...@apache.org>.
deemoliu commented on code in PR #12383:
URL: https://github.com/apache/pinot/pull/12383#discussion_r1483638578
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -660,8 +666,9 @@ protected void doTakeSnapshot() {
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId,
ServerGauge.UPSERT_PRIMARY_KEYS_IN_SNAPSHOT_COUNT, numPrimaryKeysInSnapshot);
_logger.info(
- "Finished taking snapshot for {} immutable segments with {} primary keys (out of {} total segments) in {}ms",
- numImmutableSegments, numPrimaryKeysInSnapshot, numTrackedSegments, System.currentTimeMillis() - startTimeMs);
+ "Finished taking snapshot for {} immutable segments with {} primary keys (out of {} total segments, "
+ + "{} consuming segments) in {} ms", numImmutableSegments, numPrimaryKeysInSnapshot, numTrackedSegments,
+ numConsumingSegments, System.currentTimeMillis() - startTimeMs);
Review Comment:
Just curious, how will numConsumingSegments in the log be 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
Re: [PR] Add upsert-snapshot timer metric [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #12383:
URL: https://github.com/apache/pinot/pull/12383#discussion_r1483644800
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -660,8 +666,9 @@ protected void doTakeSnapshot() {
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId,
ServerGauge.UPSERT_PRIMARY_KEYS_IN_SNAPSHOT_COUNT, numPrimaryKeysInSnapshot);
_logger.info(
- "Finished taking snapshot for {} immutable segments with {} primary keys (out of {} total segments) in {}ms",
- numImmutableSegments, numPrimaryKeysInSnapshot, numTrackedSegments, System.currentTimeMillis() - startTimeMs);
+ "Finished taking snapshot for {} immutable segments with {} primary keys (out of {} total segments, "
+ + "{} consuming segments) in {} ms", numImmutableSegments, numPrimaryKeysInSnapshot, numTrackedSegments,
+ numConsumingSegments, System.currentTimeMillis() - startTimeMs);
Review Comment:
Right now, it's confusing to differentiate if the segment for which the snapshot didn't come is the consuming one or there was an actual error. This helps us differentiate. The message currently is:
```
Finished taking snapshot for 120 immutable segments with 7794903 primary keys (out of 121 total segments) in 2343ms
```
Here the 1 segment missed is usually consuming segment. Either we can add consuming segment info or subtract numConsumingSegment from total segments. Anything works.
--
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