Minor Realtime Segment Commit Upload Improvements #10725
Conversation
ankitsultana
commented
May 5, 2023
•
ankitsultana
commented
- Allow configuring timeout for segment uploader to deep-store. This is a backwards incompatible change since the default value will change from 10s to 300s. This is done to keep the config for upload timeout same.
- Track latency for uploading segments from the server.
- Track how many segment uploads timeout, fail or succeed.
- Track how many segments have missing deep-store links.
Codecov Report
@@ Coverage Diff @@
## master #10725 +/- ##
=============================================
+ Coverage 27.51% 68.63% +41.12%
- Complexity 58 6464 +6406
=============================================
Files 2127 2143 +16
Lines 114728 115193 +465
Branches 17296 17354 +58
=============================================
+ Hits 31563 79062 +47499
+ Misses 80019 30553 -49466
- Partials 3146 5578 +2432
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1589 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ankitsultana
changed the title
| _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); |
There was a problem hiding this comment.
@ankitsultana Should we also emit metric for segment upload success / failure overall? This is not related to this PR.
There was a problem hiding this comment.
Yeah you are right. Done. Also added these metrics for Server2Controller segment uploader.
| if (StringUtils.isBlank(committingSegmentDescriptor.getSegmentLocation())) { | ||
| _controllerMetrics.addMeteredTableValue(realtimeTableName, ControllerMeter.SEGMENT_MISSING_DEEP_STORE_LINK, 1); | ||
| } |
There was a problem hiding this 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?
There was a problem hiding this 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.
