You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tarun11Mavani (via GitHub)" <gi...@apache.org> on 2025/12/12 08:53:08 UTC
[PR] Set merging segment creation time = max(creationTime) + 1 [pinot]
tarun11Mavani opened a new pull request, #17356:
URL: https://github.com/apache/pinot/pull/17356
### Problem
During upsert compact merge operations, merged segment are created with the same creation time as the maximum creation time of the merging segments. If the merging segment are UPLOADED segment (merged earlier), they share the same create time as merged segment. This means that records in segment with highest creation time is not replaced due to the tie-breaking logic in [shouldReplaceOnComparisonTie](https://github.com/apache/pinot/blob/52db36c816f91ef8887fddd0beade5d169824296/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java#L518).
Not replacing records from this segment could lead to dataloss as discussed in in #17337.
### Solution
We set the creation time of merged segment = max(creation time of all segment) + 1.
This ensures that the merging segment takes priority and all records in existing segment are replaced with records in new merged segment.
### Test
Tested in a test cluster. Verified that the new merging segment has the creation time as expected. Validated that all records from merging segment were replaced with merged segment.
All compacted segments were deleted successfully in next task iteration.
--
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] Set merging segment creation time = max(creationTime) + 1 [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17356:
URL: https://github.com/apache/pinot/pull/17356#discussion_r2613580113
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -133,7 +133,9 @@ protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig,
}
// create new UploadedRealtimeSegment
- segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments);
+ //Setting the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
+ // segments are replaced with new merged segment
+ segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments+1);
Review Comment:
segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments + 1);
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -133,7 +133,9 @@ protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig,
}
// create new UploadedRealtimeSegment
- segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments);
+ //Setting the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
+ // segments are replaced with new merged segment
+ segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments+1);
Review Comment:
segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments + 1);
--
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] Set merging segment creation time = max(creationTime) + 1 [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17356:
URL: https://github.com/apache/pinot/pull/17356#discussion_r2613582121
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -133,7 +133,9 @@ protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig,
}
// create new UploadedRealtimeSegment
- segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments);
+ //Setting the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
+ // segments are replaced with new merged segment
+ segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments+1);
Review Comment:
```suggestion
segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments + 1);
```
--
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] Set merging segment creation time = max(creationTime) + 1 [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17356:
URL: https://github.com/apache/pinot/pull/17356#issuecomment-3645697665
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17356?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.59%. Comparing base ([`3211115`](https://app.codecov.io/gh/apache/pinot/commit/321111595dfae6828ce3a73513355bdcb6181d1d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`582c82b`](https://app.codecov.io/gh/apache/pinot/commit/582c82bb72c4545464a3a7a257541aa1e22bb339?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 463 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (3211115) and HEAD (582c82b). Click for more details.
>
> <details><summary>HEAD has 32 uploads less than BASE</summary>
>
>| Flag | BASE (3211115) | HEAD (582c82b) |
>|------|------|------|
>|java-21|4|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|9|1|
>|unittests2|2|0|
>|java-11|5|0|
>|integration|5|0|
>|integration1|2|0|
>|custom-integration1|2|0|
>|integration2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17356 +/- ##
============================================
- Coverage 63.40% 55.59% -7.82%
+ Complexity 1383 702 -681
============================================
Files 3042 2459 -583
Lines 177773 138795 -38978
Branches 27246 22131 -5115
============================================
- Hits 112721 77162 -35559
+ Misses 56402 55116 -1286
+ Partials 8650 6517 -2133
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17356/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/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17356/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/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.59% <ø> (-7.79%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.59% <ø> (-7.82%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.59% <ø> (-7.81%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.59% <ø> (-0.89%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17356/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/17356?dropdown=coverage&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).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] Set merging segment creation time = max(creationTime) + 1 [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17356:
URL: https://github.com/apache/pinot/pull/17356#discussion_r2613574837
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -133,7 +133,9 @@ protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig,
}
// create new UploadedRealtimeSegment
- segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments);
+ //Setting the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
+ // segments are replaced with new merged segment
+ segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments+1);
Review Comment:
Add whitespace around the '+' operator for better readability. The expression should be written as `maxCreationTimeOfMergingSegments + 1`.
```suggestion
segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments + 1);
```
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -133,7 +133,9 @@ protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig,
}
// create new UploadedRealtimeSegment
- segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments);
+ //Setting the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
Review Comment:
Corrected 'that the all' to 'that all'.
```suggestion
//Setting the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that all records in merging
```
--
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] Set merging segment creation time = max(creationTime) + 1 [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17356:
URL: https://github.com/apache/pinot/pull/17356#discussion_r2613587973
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -133,7 +133,9 @@ protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig,
}
// create new UploadedRealtimeSegment
- segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments);
+ // set the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
Review Comment:
```suggestion
// set the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that all records in merging
```
--
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] Set merging segment creation time = max(creationTime) + 1 [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17356:
URL: https://github.com/apache/pinot/pull/17356
--
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] Set merging segment creation time = max(creationTime) + 1 [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17356:
URL: https://github.com/apache/pinot/pull/17356#discussion_r2613585154
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompactmerge/UpsertCompactMergeTaskExecutor.java:
##########
@@ -133,7 +133,9 @@ protected List<SegmentConversionResult> convert(PinotTaskConfig pinotTaskConfig,
}
// create new UploadedRealtimeSegment
- segmentProcessorConfigBuilder.setCustomCreationTime(maxCreationTimeOfMergingSegments);
+ //Setting the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
Review Comment:
```suggestion
// set the creation time to maxCreationTimeOfMergingSegments + 1 to ensure that the all records in merging
```
--
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