You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2025/04/08 17:41:02 UTC
[PR] Fix committing segments list [pinot]
Jackie-Jiang opened a new pull request, #15495:
URL: https://github.com/apache/pinot/pull/15495
- Properly handle `UPLOADED` segment
- When syncing committing segments, check the segments again to handle recent update
--
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] Fix committing segments list [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #15495:
URL: https://github.com/apache/pinot/pull/15495#discussion_r2036418450
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2573,66 +2573,37 @@ public Set<String> getSegmentsYetToBeCommitted(String tableNameWithType, Set<Str
* @param committingSegments List of new segment names that are currently in COMMITTING state.
* If null, returns true without making any changes to the existing list
* @return true if the synchronization succeeds, false if there's a failure in updating ZooKeeper
- * @see #getCommittingSegments for the logic that filters out segments no longer in COMMITTING state
*/
- public boolean syncCommittingSegments(String realtimeTableName, @NotNull List<String> committingSegments) {
+ public boolean syncCommittingSegments(String realtimeTableName, List<String> committingSegments) {
+ String pauselessDebugMetadataPath =
+ ZKMetadataProvider.constructPropertyStorePathForPauselessDebugMetadata(realtimeTableName);
return updateCommittingSegmentsList(realtimeTableName, () -> {
- String committingSegmentsListPath =
- ZKMetadataProvider.constructPropertyStorePathForPauselessDebugMetadata(realtimeTableName);
-
// Fetch the committing segments record from the property store.
Stat stat = new Stat();
- ZNRecord znRecord = _propertyStore.get(committingSegmentsListPath, stat, AccessOption.PERSISTENT);
+ ZNRecord znRecord = _propertyStore.get(pauselessDebugMetadataPath, stat, AccessOption.PERSISTENT);
- // empty ZN record for the table
+ // Create ZN record if it doesn't exist
if (znRecord == null) {
znRecord = new ZNRecord(realtimeTableName);
znRecord.setListField(COMMITTING_SEGMENTS, committingSegments);
- return _propertyStore.create(committingSegmentsListPath, znRecord, AccessOption.PERSISTENT);
+ return _propertyStore.create(pauselessDebugMetadataPath, znRecord, AccessOption.PERSISTENT);
}
- Set<String> mergedSegments = new HashSet<>(committingSegments);
- // Get segments that are present in the list and are still in COMMITTING status
- List<String> existingSegments =
- getCommittingSegments(realtimeTableName, znRecord.getListField(COMMITTING_SEGMENTS));
- if (existingSegments != null) {
- mergedSegments.addAll(existingSegments);
+ // Check ZK metadata again to get the latest list of committing segments
+ List<String> committingSegmentsFromPropertyStore = znRecord.getListField(COMMITTING_SEGMENTS);
Review Comment:
nit: maybe we can call it `commitSegmentsSnapshot`
--
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] Fix committing segments list [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #15495:
URL: https://github.com/apache/pinot/pull/15495#discussion_r2036418450
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2573,66 +2573,37 @@ public Set<String> getSegmentsYetToBeCommitted(String tableNameWithType, Set<Str
* @param committingSegments List of new segment names that are currently in COMMITTING state.
* If null, returns true without making any changes to the existing list
* @return true if the synchronization succeeds, false if there's a failure in updating ZooKeeper
- * @see #getCommittingSegments for the logic that filters out segments no longer in COMMITTING state
*/
- public boolean syncCommittingSegments(String realtimeTableName, @NotNull List<String> committingSegments) {
+ public boolean syncCommittingSegments(String realtimeTableName, List<String> committingSegments) {
+ String pauselessDebugMetadataPath =
+ ZKMetadataProvider.constructPropertyStorePathForPauselessDebugMetadata(realtimeTableName);
return updateCommittingSegmentsList(realtimeTableName, () -> {
- String committingSegmentsListPath =
- ZKMetadataProvider.constructPropertyStorePathForPauselessDebugMetadata(realtimeTableName);
-
// Fetch the committing segments record from the property store.
Stat stat = new Stat();
- ZNRecord znRecord = _propertyStore.get(committingSegmentsListPath, stat, AccessOption.PERSISTENT);
+ ZNRecord znRecord = _propertyStore.get(pauselessDebugMetadataPath, stat, AccessOption.PERSISTENT);
- // empty ZN record for the table
+ // Create ZN record if it doesn't exist
if (znRecord == null) {
znRecord = new ZNRecord(realtimeTableName);
znRecord.setListField(COMMITTING_SEGMENTS, committingSegments);
- return _propertyStore.create(committingSegmentsListPath, znRecord, AccessOption.PERSISTENT);
+ return _propertyStore.create(pauselessDebugMetadataPath, znRecord, AccessOption.PERSISTENT);
}
- Set<String> mergedSegments = new HashSet<>(committingSegments);
- // Get segments that are present in the list and are still in COMMITTING status
- List<String> existingSegments =
- getCommittingSegments(realtimeTableName, znRecord.getListField(COMMITTING_SEGMENTS));
- if (existingSegments != null) {
- mergedSegments.addAll(existingSegments);
+ // Check ZK metadata again to get the latest list of committing segments
+ List<String> committingSegmentsFromPropertyStore = znRecord.getListField(COMMITTING_SEGMENTS);
Review Comment:
nit: maybe we can rename it from `committingSegmentsFromPropertyStore` to `commitSegmentsSnapshot`
--
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] Fix committing segments list [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15495:
URL: https://github.com/apache/pinot/pull/15495#issuecomment-2787321180
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15495?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 55.82%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`637e77a`)](https://app.codecov.io/gh/apache/pinot/commit/637e77a88683f726c3790ac4677837041d98361d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 2017 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (637e77a). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (637e77a) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|1|
>|unittests1|2|1|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15495 +/- ##
============================================
- Coverage 61.75% 55.82% -5.93%
- Complexity 207 681 +474
============================================
Files 2436 2223 -213
Lines 133233 120062 -13171
Branches 20636 19185 -1451
============================================
- Hits 82274 67030 -15244
- Misses 44911 47502 +2591
+ Partials 6048 5530 -518
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15495/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/15495/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/15495/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/15495/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/15495/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/15495/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/15495/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.82% <ø> (-5.80%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15495/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.82% <ø> (-5.92%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15495/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15495/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.82% <ø> (-5.93%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15495/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.82% <ø> (-5.92%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15495/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.82% <ø> (+8.93%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15495/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/15495?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>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :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] Fix committing segments list [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps commented on code in PR #15495:
URL: https://github.com/apache/pinot/pull/15495#discussion_r2036420411
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2573,66 +2573,37 @@ public Set<String> getSegmentsYetToBeCommitted(String tableNameWithType, Set<Str
* @param committingSegments List of new segment names that are currently in COMMITTING state.
* If null, returns true without making any changes to the existing list
* @return true if the synchronization succeeds, false if there's a failure in updating ZooKeeper
- * @see #getCommittingSegments for the logic that filters out segments no longer in COMMITTING state
*/
- public boolean syncCommittingSegments(String realtimeTableName, @NotNull List<String> committingSegments) {
+ public boolean syncCommittingSegments(String realtimeTableName, List<String> committingSegments) {
Review Comment:
Is this method thread safe? What happens when we get two parallel calls to this method with same table but different comittingSegments?
--
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] Fix committing segments list [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15495:
URL: https://github.com/apache/pinot/pull/15495#discussion_r2037910523
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java:
##########
@@ -2573,66 +2573,37 @@ public Set<String> getSegmentsYetToBeCommitted(String tableNameWithType, Set<Str
* @param committingSegments List of new segment names that are currently in COMMITTING state.
* If null, returns true without making any changes to the existing list
* @return true if the synchronization succeeds, false if there's a failure in updating ZooKeeper
- * @see #getCommittingSegments for the logic that filters out segments no longer in COMMITTING state
*/
- public boolean syncCommittingSegments(String realtimeTableName, @NotNull List<String> committingSegments) {
+ public boolean syncCommittingSegments(String realtimeTableName, List<String> committingSegments) {
Review Comment:
It should be thread safe because we perform version check before posting the metadata to ZK
--
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] Fix committing segments list [pinot]
Posted by "KKcorps (via GitHub)" <gi...@apache.org>.
KKcorps merged PR #15495:
URL: https://github.com/apache/pinot/pull/15495
--
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