Upsert small segment merger task in minions #14477

tibrewalpratik17 · 2024-11-18T10:23:00Z

PR related to the PEP request: #14305

Here, we are adding a new minion task to merge small segments in an upsert table. More implementation details in the design doc of the linked issue.

Test plan: Enabled this in one of infinite retention tables in Uber. The tables had ~35k segments initially and after enabling this task for ~2 days we were able to reach ~2k segments. The curve also flattens post reaching ~2k segments. We are using the default configs of this task and the table is generating ~500 segments daily. See attached screenshot.

Few details:

Unlike upsert compaction, we are enforcing enableSnapshot to be enabled for running this task. This makes sense from correctness perspective.
The new segment name formed example: compactmerged__table__5__1731976847331__0
The creation time of the new segment in ZK = max creation time in ZK of merged segments. Note the segment name doesn't have the index creation time value but the task running time.
The code merges LLC segment and UploadedRealtime segments which is desired.
Right now, we process one merging subtask per partition per task run. We will explore later of allowing multiple merging subtasks per partition per task run.

codecov-commenter · 2024-11-18T11:00:05Z

Codecov Report

Attention: Patch coverage is 21.93548% with 242 lines in your changes missing coverage. Please review.

Project coverage is 63.98%. Comparing base (59551e4) to head (6d77e17).
Report is 1455 commits behind head on master.

Files with missing lines	Patch %	Lines
...tcompactmerge/UpsertCompactMergeTaskGenerator.java	20.39%	157 Missing and 3 partials ⚠️
...rtcompactmerge/UpsertCompactMergeTaskExecutor.java	24.13%	65 Missing and 1 partial ⚠️
...ctmerge/UpsertCompactMergeTaskExecutorFactory.java	0.00%	6 Missing ⚠️
...t/processing/framework/SegmentProcessorConfig.java	44.44%	4 Missing and 1 partial ⚠️
...rocessing/framework/SegmentProcessorFramework.java	50.00%	1 Missing and 1 partial ⚠️
...UpsertCompactMergeTaskProgressObserverFactory.java	0.00%	2 Missing ⚠️
.../org/apache/pinot/core/common/MinionConstants.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14477      +/-   ##
============================================
+ Coverage     61.75%   63.98%   +2.23%     
- Complexity      207     1600    +1393     
============================================
  Files          2436     2696     +260     
  Lines        133233   148389   +15156     
  Branches      20636    22740    +2104     
============================================
+ Hits          82274    94946   +12672     
- Misses        44911    46476    +1565     
- Partials       6048     6967     +919

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.95% <21.93%> (+2.24%)`	⬆️
java-21	`63.86% <21.93%> (+2.24%)`	⬆️
skip-bytebuffers-false	`63.96% <21.93%> (+2.22%)`	⬆️
skip-bytebuffers-true	`63.84% <21.93%> (+36.12%)`	⬆️
temurin	`63.98% <21.93%> (+2.23%)`	⬆️
unittests	`63.98% <21.93%> (+2.23%)`	⬆️
unittests1	`56.21% <42.85%> (+9.32%)`	⬆️
unittests2	`34.51% <20.00%> (+6.78%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tibrewalpratik17 · 2024-11-19T20:36:36Z

Marking it ready for review for early feedback!

pinot-core/src/main/java/org/apache/pinot/core/common/MinionConstants.java

.../org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java

...org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java

ankitsultana · 2024-12-04T21:11:07Z

pinot-core/src/main/java/org/apache/pinot/core/common/MinionConstants.java

@@ -199,4 +200,59 @@ public static class UpsertCompactionTask {
     */
    public static final String NUM_SEGMENTS_BATCH_PER_SERVER_REQUEST = "numSegmentsBatchPerServerRequest";
  }
+
+  public static class UpsertCompactMergeTask {


broad question: say we run both merge compaction and the segment refresh task for a table. How do we ensure that there aren't any race conditions, because it could be that the two tasks end up processing the same set of segments concurrently.

So with merge-compaction we would end up creating a new segment and refresh would just go and do a replaceSegment flow in metadata-manager. For refresh of LLC segment we should be good as the newly generated UploadedRealtimeSegment would override the keys which it should.
For refresh of UploadedRealtimeSegment, there's an issue right now where we use creation time to resolve for the latest uploaded segment, now there can be an edge case where creation time is same for both then whichever gets refreshed / uploaded later would dominate. But overall if you see it doesn't matter in the broad sense because for upserts we would still point to just one record per key.

Correctness wise we should be fine I guess, but this will still impede or impact the Segment Refresh task. Can you create a separate issue for this with some context on the problem so we could pick it up later?

...src/main/java/org/apache/pinot/core/segment/processing/framework/SegmentProcessorConfig.java

klsince · 2024-12-05T23:20:45Z

...org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java

+  private static final String DEFAULT_BUFFER_PERIOD = "2d";
+  private static final int DEFAULT_NUM_SEGMENTS_BATCH_PER_SERVER_REQUEST = 500;
+
+  public static class SegmentMergerMetadata {


private class or VisibleForTests?

...org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java

klsince · 2024-12-05T23:26:34Z

.../org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java

+    LOGGER.info("Finished task: {} with configs: {}. Total time: {}ms", taskType, configs, (endMillis - startMillis));
+
+    List<SegmentConversionResult> results = new ArrayList<>();
+    for (File outputSegmentDir : outputSegmentDirs) {


just to be sure, could one task generate multiple new segments or just one? If multiple segments could be created, then how to handle cases when some segments failed to get uploaded.

Current implementation ensures generating one segment. But if you think about it even if we generate multiple segments, we don't need to think about rollback much. It's just that whatever segments got uploaded, validDocIds will be uploaded for those and the corresponding segments would be discarded as their docs will be invalid (assuming takeSnapshot has run once).

I see.

For M:N merging, I think the computation of alreadyMergedSegments might need some extensions, as you may want to retry some of the input segments, instead of skipping them if they are in some segment's custom map.

For M:N merging, I think the computation of alreadyMergedSegments might need some extensions, as you may want to retry some of the input segments, instead of skipping them if they are in some segment's custom map.

Yeah or we ensure always selecting complete 'x' segments while creating N segments such that sum over x = M. For the sake of simplicity, we are resolving this in task-generation itself currently ensuring to generate multiple X:1 subtasks instead.

...org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java

klsince · 2024-12-09T19:26:05Z

.../org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java

+    LOGGER.info("Finished task: {} with configs: {}. Total time: {}ms", taskType, configs, (endMillis - startMillis));
+
+    List<SegmentConversionResult> results = new ArrayList<>();
+    for (File outputSegmentDir : outputSegmentDirs) {


I see.

For M:N merging, I think the computation of alreadyMergedSegments might need some extensions, as you may want to retry some of the input segments, instead of skipping them if they are in some segment's custom map.

ankitsultana · 2024-12-10T23:43:57Z

.../org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java

+    // get the max creation time of the small segments. This will be the index creation time for the new segment.
+    Optional<Long> maxCreationTimeOfMergingSegments = segmentMetadataList.stream().map(
+        SegmentMetadataImpl::getIndexCreationTime).reduce(Long::max);
+    if (maxCreationTimeOfMergingSegments.isEmpty()) {


Just to confirm: numInputSegments is always greater than 0 right?

Yes we have added the check in generator code to ensure numInputSegments > 1

ankitsultana · 2024-12-10T23:50:10Z

...org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java

+      int partitionID = entry.getKey();
+      List<SegmentMergerMetadata> segments = entry.getValue();
+      // task config thresholds
+      // TODO add output segment size as one of the thresholds


Can we create a issue for this for tracking?

klsince

LGTM

...org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskGenerator.java

tibrewalpratik17 added feature release-notes Referenced by PRs that need attention when compiling the next release notes upsert minion documentation labels Nov 18, 2024

tibrewalpratik17 force-pushed the upsert_compact_merge branch 3 times, most recently from dc1df38 to 64cd7d6 Compare November 19, 2024 13:41

tibrewalpratik17 marked this pull request as ready for review November 19, 2024 20:36

tibrewalpratik17 requested review from klsince, ankitsultana and Jackie-Jiang November 19, 2024 20:36

ankitsultana reviewed Dec 4, 2024

View reviewed changes

klsince reviewed Dec 5, 2024

View reviewed changes

tibrewalpratik17 force-pushed the upsert_compact_merge branch from 83f52a8 to ed2f8ea Compare December 6, 2024 03:11

klsince reviewed Dec 9, 2024

View reviewed changes

tibrewalpratik17 added 2 commits December 11, 2024 00:49

Scaffold small segment merger task in minions

7297c34

address comments

8d519b4

tibrewalpratik17 force-pushed the upsert_compact_merge branch from 96c5e17 to 3487e06 Compare December 10, 2024 19:19

address comments 2

f82aa93

tibrewalpratik17 force-pushed the upsert_compact_merge branch from 3487e06 to f82aa93 Compare December 10, 2024 20:03

ankitsultana approved these changes Dec 10, 2024

View reviewed changes

This was referenced Dec 11, 2024

SegmentRefresh task compatibility with UpsertCompactMerge task #14633

Open

Support for size based minion UpsertCompactMerge task #14634

Open

klsince approved these changes Dec 11, 2024

View reviewed changes

address comments refactor

6d77e17

ankitsultana merged commit 626c45d into apache:master Dec 12, 2024
21 checks passed

tibrewalpratik17 mentioned this pull request Dec 16, 2024

Upsert Small Segment Merger Task [Minion] #14305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upsert small segment merger task in minions #14477

Upsert small segment merger task in minions #14477

tibrewalpratik17 commented Nov 18, 2024 •

edited

Loading

codecov-commenter commented Nov 18, 2024 •

edited

Loading

tibrewalpratik17 commented Nov 19, 2024

ankitsultana Dec 4, 2024

tibrewalpratik17 Dec 5, 2024

ankitsultana Dec 10, 2024

tibrewalpratik17 Dec 11, 2024

klsince Dec 5, 2024

klsince Dec 5, 2024

tibrewalpratik17 Dec 6, 2024

klsince Dec 9, 2024

tibrewalpratik17 Dec 10, 2024

klsince Dec 9, 2024

ankitsultana Dec 10, 2024

tibrewalpratik17 Dec 11, 2024

ankitsultana Dec 10, 2024

tibrewalpratik17 Dec 11, 2024

klsince left a comment

Upsert small segment merger task in minions #14477

Upsert small segment merger task in minions #14477

Conversation

tibrewalpratik17 commented Nov 18, 2024 • edited Loading

codecov-commenter commented Nov 18, 2024 • edited Loading

Codecov Report

tibrewalpratik17 commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klsince left a comment

Choose a reason for hiding this comment

tibrewalpratik17 commented Nov 18, 2024 •

edited

Loading

codecov-commenter commented Nov 18, 2024 •

edited

Loading