You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ankitsultana (via GitHub)" <gi...@apache.org> on 2023/05/05 05:10:51 UTC
[GitHub] [pinot] ankitsultana opened a new pull request, #10725: Realtime Segment Commit Upload
ankitsultana opened a new pull request, #10725:
URL: https://github.com/apache/pinot/pull/10725
TBD
--
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] tibrewalpratik17 commented on a diff in pull request #10725: Minor Realtime Segment Commit Upload Improvements
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10725:
URL: https://github.com/apache/pinot/pull/10725#discussion_r1187719895
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PinotFSSegmentUploader.java:
##########
@@ -96,9 +96,8 @@ public URI uploadSegment(File segmentFile, LLCSegmentName segmentName) {
LOGGER.info("Interrupted while waiting for segment upload of {} to {}.", segmentName, _segmentStoreUriStr);
Thread.currentThread().interrupt();
} catch (TimeoutException e) {
- _serverMetrics.addMeteredTableValue(segmentName.getTableName(), ServerMeter.SEGMENT_UPLOAD_TIMEOUT, 1);
- LOGGER.warn("Timed out waiting to upload segment: {} for table: {}",
- segmentName.getSegmentName(), segmentName.getTableName());
+ _serverMetrics.addMeteredTableValue(rawTableName, ServerMeter.SEGMENT_UPLOAD_TIMEOUT, 1);
Review Comment:
@ankitsultana Should we also emit metric for segment upload success / failure overall? This is not related to this PR.
--
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] tibrewalpratik17 commented on a diff in pull request #10725: Minor Realtime Segment Commit Upload Improvements
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10725:
URL: https://github.com/apache/pinot/pull/10725#discussion_r1187828890
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -606,6 +607,10 @@ private void commitSegmentMetadataInternal(String realtimeTableName,
// Trigger the metadata event notifier
_metadataEventNotifierFactory.create().notifyOnSegmentFlush(tableConfig);
+
+ if (StringUtils.isBlank(committingSegmentDescriptor.getSegmentLocation())) {
+ _controllerMetrics.addMeteredTableValue(realtimeTableName, ControllerMeter.SEGMENT_MISSING_DEEP_STORE_LINK, 1);
+ }
Review Comment:
Hmm makes sense!
--
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] tibrewalpratik17 commented on a diff in pull request #10725: Minor Realtime Segment Commit Upload Improvements
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #10725:
URL: https://github.com/apache/pinot/pull/10725#discussion_r1187756006
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -606,6 +607,10 @@ private void commitSegmentMetadataInternal(String realtimeTableName,
// Trigger the metadata event notifier
_metadataEventNotifierFactory.create().notifyOnSegmentFlush(tableConfig);
+
+ if (StringUtils.isBlank(committingSegmentDescriptor.getSegmentLocation())) {
+ _controllerMetrics.addMeteredTableValue(realtimeTableName, ControllerMeter.SEGMENT_MISSING_DEEP_STORE_LINK, 1);
+ }
Review Comment:
@ankitsultana will this not be equal to the failure metric which we are emitting from `SegmentUploader` classes? Can there be a scenario where these 2 differ? If not, do we need this?
--
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 #10725: Realtime Segment Commit Upload
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10725:
URL: https://github.com/apache/pinot/pull/10725#issuecomment-1535725611
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10725?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 [#10725](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d1d9763) into [master](https://app.codecov.io/gh/apache/pinot/commit/815cd6ebb7f2236b127a4b7a7f8d59bea2c9a1cc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (815cd6e) will **decrease** coverage by `50.56%`.
> The diff coverage is `13.33%`.
```diff
@@ Coverage Diff @@
## master #10725 +/- ##
=============================================
- Coverage 64.38% 13.82% -50.56%
+ Complexity 6441 439 -6002
=============================================
Files 2068 2068
Lines 111794 111806 +12
Branches 16956 16957 +1
=============================================
- Hits 71974 15459 -56515
- Misses 34646 95084 +60438
+ Partials 5174 1263 -3911
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `13.82% <13.33%> (-0.01%)` | :arrow_down: |
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://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...a/org/apache/pinot/common/metrics/ServerTimer.java](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJUaW1lci5qYXZh) | `0.00% <0.00%> (-84.62%)` | :arrow_down: |
| [.../data/manager/realtime/PinotFSSegmentUploader.java](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGlub3RGU1NlZ21lbnRVcGxvYWRlci5qYXZh) | `0.00% <0.00%> (-75.76%)` | :arrow_down: |
| [...data/manager/realtime/SegmentCommitterFactory.java](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VnbWVudENvbW1pdHRlckZhY3RvcnkuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ger/realtime/Server2ControllerSegmentUploader.java](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvU2VydmVyMkNvbnRyb2xsZXJTZWdtZW50VXBsb2FkZXIuamF2YQ==) | `0.00% <0.00%> (-72.42%)` | :arrow_down: |
| [...altime/ServerSegmentCompletionProtocolHandler.java](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VydmVyL3JlYWx0aW1lL1NlcnZlclNlZ21lbnRDb21wbGV0aW9uUHJvdG9jb2xIYW5kbGVyLmphdmE=) | `0.00% <ø> (-15.10%)` | :arrow_down: |
| [.../core/realtime/PinotLLCRealtimeSegmentManager.java](https://app.codecov.io/gh/apache/pinot/pull/10725?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90TExDUmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `60.81% <100.00%> (+0.11%)` | :arrow_up: |
... and [1412 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10725/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 pull request #10725: Minor Realtime Segment Commit Upload Improvements
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10725:
URL: https://github.com/apache/pinot/pull/10725#issuecomment-1539316444
tagging @npawar and @klsince to take a 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] ankitsultana commented on a diff in pull request #10725: Minor Realtime Segment Commit Upload Improvements
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10725:
URL: https://github.com/apache/pinot/pull/10725#discussion_r1187746195
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PinotFSSegmentUploader.java:
##########
@@ -96,9 +96,8 @@ public URI uploadSegment(File segmentFile, LLCSegmentName segmentName) {
LOGGER.info("Interrupted while waiting for segment upload of {} to {}.", segmentName, _segmentStoreUriStr);
Thread.currentThread().interrupt();
} catch (TimeoutException e) {
- _serverMetrics.addMeteredTableValue(segmentName.getTableName(), ServerMeter.SEGMENT_UPLOAD_TIMEOUT, 1);
- LOGGER.warn("Timed out waiting to upload segment: {} for table: {}",
- segmentName.getSegmentName(), segmentName.getTableName());
+ _serverMetrics.addMeteredTableValue(rawTableName, ServerMeter.SEGMENT_UPLOAD_TIMEOUT, 1);
Review Comment:
Yeah you are right. Done. Also added these metrics for Server2Controller segment uploader.
--
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] ankitsultana commented on a diff in pull request #10725: Minor Realtime Segment Commit Upload Improvements
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10725:
URL: https://github.com/apache/pinot/pull/10725#discussion_r1187828148
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -606,6 +607,10 @@ private void commitSegmentMetadataInternal(String realtimeTableName,
// Trigger the metadata event notifier
_metadataEventNotifierFactory.create().notifyOnSegmentFlush(tableConfig);
+
+ if (StringUtils.isBlank(committingSegmentDescriptor.getSegmentLocation())) {
+ _controllerMetrics.addMeteredTableValue(realtimeTableName, ControllerMeter.SEGMENT_MISSING_DEEP_STORE_LINK, 1);
+ }
Review Comment:
It can be different. In Split commit servers upload the file to a temp location, and the file is atomically moved to the final location in the controller. It could be that different replicas try to upload the segment to the deep-store.
--
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] chenboat merged pull request #10725: Minor Realtime Segment Commit Upload Improvements
Posted by "chenboat (via GitHub)" <gi...@apache.org>.
chenboat merged PR #10725:
URL: https://github.com/apache/pinot/pull/10725
--
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