You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/06/13 18:04:21 UTC
[GitHub] [pinot] jtao15 opened a new pull request, #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
jtao15 opened a new pull request, #8838:
URL: https://github.com/apache/pinot/pull/8838
This pr should solve the followings:
1. It's possible to violate the 2X storage usage because the segment cleanup in startReplaceSegment is async. E.g. a refresh table has 2 snapshots already. The the 3rd push got success response from startReplaceSegment api and starts to upload, but the cleanup for the 1st snapshot fails.
2. In case of rerunning the failed push, there's a race condition between retention manager/revertReplace and startReplace.
```
For example, the retention manager can delete the new uploaded s3 and s4 because the delete is async in retention manager.
Initial state:
lineageEntry1: { segmentsFrom: [s1, s2], segmentsTo: [s3, s4], status: REVERTED/IN_PROGRESS > 24h}
Running startReplace:
lineageEntry1: { segmentsFrom: [s1, s2], segmentsTo: [], status: REVERTED}
lineageEntry2: { segmentsFrom: [s1, s2], segmentsTo: [s3, s4], status: IN_PROGRESS}
```
This pr should solve the race condition by waiting for the clean up for s3 and s4 before completing startReplace so the client side hasn't upload s3 and s4 yet.
--
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] jtao15 commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jtao15 commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r895994262
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3163,6 +3165,7 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
if (!segmentsToCleanUp.isEmpty()) {
LOGGER.info("Cleaning up the segments while startReplaceSegments: {}", segmentsToCleanUp);
deleteSegments(tableNameWithType, segmentsToCleanUp);
+ waitForSegmentsToDelete(tableNameWithType, segmentsToCleanUp, SEGMENT_CLEANUP_TIMEOUT_MS);
Review Comment:
This is more for correctness. By doing so, it guarantees at most 2 snapshots are stored and we will not violate the storage quota. It also helps the race condition mentioned in the description. The client side may need to change the socket timeout based on this change.
--
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] Jackie-Jiang commented on pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8838:
URL: https://github.com/apache/pinot/pull/8838#issuecomment-1150412615
cc @klsince to take a look at the change
--
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] klsince commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r900190739
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3163,6 +3165,7 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
if (!segmentsToCleanUp.isEmpty()) {
LOGGER.info("Cleaning up the segments while startReplaceSegments: {}", segmentsToCleanUp);
deleteSegments(tableNameWithType, segmentsToCleanUp);
+ waitForSegmentsToDelete(tableNameWithType, segmentsToCleanUp, SEGMENT_CLEANUP_TIMEOUT_MS);
Review Comment:
@snleee I haven't seen long delay on segment deletion, but noticed some delay when using SegmentLineage thus having suggested a feature flag. But good to know the `forceClean` can be used for that purpose.
--
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] snleee commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r899415424
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3163,6 +3165,7 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
if (!segmentsToCleanUp.isEmpty()) {
LOGGER.info("Cleaning up the segments while startReplaceSegments: {}", segmentsToCleanUp);
deleteSegments(tableNameWithType, segmentsToCleanUp);
+ waitForSegmentsToDelete(tableNameWithType, segmentsToCleanUp, SEGMENT_CLEANUP_TIMEOUT_MS);
Review Comment:
@klsince Essentially, because of async deletion, there can be a short duration where 3 data snapshots (old, current, new) exist in the table and this can fill up the disk space pretty easily. To avoid that, we would want to make sure to finish "clean up" before we allow the new data push.
@jtao15 I actually think that we should consider making this optional. For instance, S3 file system implementation needs to copy data and then delete the previous one for `doMove()`. In this case, it will take a long time to delete segments.
@klsince Have you seen any long delay in segment deletion in your deployments? We internally use NFS so `doMove` is a very cheap operation.
--
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] snleee commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r899415424
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3163,6 +3165,7 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
if (!segmentsToCleanUp.isEmpty()) {
LOGGER.info("Cleaning up the segments while startReplaceSegments: {}", segmentsToCleanUp);
deleteSegments(tableNameWithType, segmentsToCleanUp);
+ waitForSegmentsToDelete(tableNameWithType, segmentsToCleanUp, SEGMENT_CLEANUP_TIMEOUT_MS);
Review Comment:
@klsince Essentially, because of async deletion, there can be a short duration where 3 data snapshots (old, current, new) exist in the table and this can fill up the disk space pretty easily. To avoid that, we would want to make sure to finish "clean up" before we allow the new data push.
@jtao15 I actually think that we should consider making this optional. For instance, S3 file system implementation needs to copy data and then delete the previous one for `doMove()`. In this case, it will take a long time to delete segments.
--
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] jtao15 closed pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jtao15 closed pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
URL: https://github.com/apache/pinot/pull/8838
--
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] jtao15 commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jtao15 commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r899622415
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3163,6 +3165,7 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
if (!segmentsToCleanUp.isEmpty()) {
LOGGER.info("Cleaning up the segments while startReplaceSegments: {}", segmentsToCleanUp);
deleteSegments(tableNameWithType, segmentsToCleanUp);
+ waitForSegmentsToDelete(tableNameWithType, segmentsToCleanUp, SEGMENT_CLEANUP_TIMEOUT_MS);
Review Comment:
@klsince I discussed with @snleee offline:
1. The deletion only happens when `forceCleanup=true`
2. Controller will fail the request to start the protocol after 20 mins, but the deletion will keep going. For the file system that takes long time to delete, the client side will finally succeed after some retries.
Based on above, we should be good to change the deletion to blocking.
--
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] snleee commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
snleee commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r891492903
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -173,6 +173,8 @@ public class PinotHelixResourceManager {
// TODO: make this configurable
public static final long EXTERNAL_VIEW_ONLINE_SEGMENTS_MAX_WAIT_MS = 10 * 60_000L; // 10 minutes
public static final long EXTERNAL_VIEW_CHECK_INTERVAL_MS = 1_000L; // 1 second
+ public static final long SEGMENT_CLEANUP_TIMEOUT_MS = 20 * 60_000L; // 20 minutes
Review Comment:
How did you come up with 20 minutes? Have we observed a case where the data deletion takes > 20minutes?
--
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] jtao15 closed pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jtao15 closed pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
URL: https://github.com/apache/pinot/pull/8838
--
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] snleee merged pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
snleee merged PR #8838:
URL: https://github.com/apache/pinot/pull/8838
--
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] jtao15 commented on pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jtao15 commented on PR #8838:
URL: https://github.com/apache/pinot/pull/8838#issuecomment-1149121397
> @jtao15 Can you add a bit more context to the description?
Added more context.
--
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 #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8838:
URL: https://github.com/apache/pinot/pull/8838#issuecomment-1147828473
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8838?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 [#8838](https://codecov.io/gh/apache/pinot/pull/8838?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cf3d86a) into [master](https://codecov.io/gh/apache/pinot/commit/e8f9d88fc3ab9ba8ca9a4e0c2dfce49c0dbbfd7b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e8f9d88) will **decrease** coverage by `42.81%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head cf3d86a differs from pull request most recent head f555b02. Consider uploading reports for the commit f555b02 to get more accurate results
```diff
@@ Coverage Diff @@
## master #8838 +/- ##
=============================================
- Coverage 68.56% 25.75% -42.82%
+ Complexity 4640 45 -4595
=============================================
Files 1741 1729 -12
Lines 91475 91130 -345
Branches 13674 13638 -36
=============================================
- Hits 62724 23469 -39255
- Misses 24363 65391 +41028
+ Partials 4388 2270 -2118
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `25.75% <0.00%> (?)` | |
| unittests1 | `?` | |
| unittests2 | `?` | |
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://codecov.io/gh/apache/pinot/pull/8838?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `31.05% <0.00%> (-34.96%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/StringUtil.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvU3RyaW5nVXRpbC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1301 more](https://codecov.io/gh/apache/pinot/pull/8838/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8838?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8838?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e8f9d88...f555b02](https://codecov.io/gh/apache/pinot/pull/8838?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] jtao15 commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jtao15 commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r891670095
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -173,6 +173,8 @@ public class PinotHelixResourceManager {
// TODO: make this configurable
public static final long EXTERNAL_VIEW_ONLINE_SEGMENTS_MAX_WAIT_MS = 10 * 60_000L; // 10 minutes
public static final long EXTERNAL_VIEW_CHECK_INTERVAL_MS = 1_000L; // 1 second
+ public static final long SEGMENT_CLEANUP_TIMEOUT_MS = 20 * 60_000L; // 20 minutes
Review Comment:
Usually the deletion should be fast, but for large multi-tenants cluster, helix can be slow for updating the externalview even if the actual state on server side has changed (> 10 mins). Segment deletion will 1. update idealstate 2. wait externalview to converge 3. remove zk metadata. I'm putting this as 20 mins by considering the helix delay.
--
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] jtao15 commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jtao15 commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r895990837
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3183,6 +3186,20 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
return segmentLineageEntryId;
}
+ private void waitForSegmentsToDelete(String tableNameWithType, List<String> segments, long timeOutInMillis)
+ throws InterruptedException {
+ LOGGER.info("Wait segments to delete. timeout = {}ms, segments = {}", timeOutInMillis, segments);
+ long endTimeMs = System.currentTimeMillis() + timeOutInMillis;
+ do {
+ if (Collections.disjoint(getSegmentsFor(tableNameWithType, false), segments)) {
+ return;
+ } else {
+ Thread.sleep(SEGMENT_CLEANUP_CHECK_INTERVAL_MS);
+ }
+ } while (System.currentTimeMillis() < endTimeMs);
+ throw new RuntimeException("Timeout while waiting for segments to be deleted");
Review Comment:
logged the table name and timeout.
We are logging the exception on `L3178`, so we don't need any other 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
[GitHub] [pinot] klsince commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r893912885
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3163,6 +3165,7 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
if (!segmentsToCleanUp.isEmpty()) {
LOGGER.info("Cleaning up the segments while startReplaceSegments: {}", segmentsToCleanUp);
deleteSegments(tableNameWithType, segmentsToCleanUp);
+ waitForSegmentsToDelete(tableNameWithType, segmentsToCleanUp, SEGMENT_CLEANUP_TIMEOUT_MS);
Review Comment:
is waitForSegmentsToDelete required for correctness or more for improvement purpose? If latter, perhaps add a feature switch to turn it on/off.
--
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] jackjlli commented on a diff in pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8838:
URL: https://github.com/apache/pinot/pull/8838#discussion_r891677547
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3183,6 +3186,20 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
return segmentLineageEntryId;
}
+ private void waitForSegmentsToDelete(String tableNameWithType, List<String> segments, long timeOutInMillis)
+ throws InterruptedException {
+ LOGGER.info("Wait segments to delete. timeout = {}ms, segments = {}", timeOutInMillis, segments);
Review Comment:
`LOGGER.info("Waiting for {} segments to delete for table: {}. timeout = {}ms, segments = {}", segments.size(), tableNameWithType, timeOutInMillis, segments)`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3183,6 +3186,20 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
return segmentLineageEntryId;
}
+ private void waitForSegmentsToDelete(String tableNameWithType, List<String> segments, long timeOutInMillis)
+ throws InterruptedException {
+ LOGGER.info("Wait segments to delete. timeout = {}ms, segments = {}", timeOutInMillis, segments);
+ long endTimeMs = System.currentTimeMillis() + timeOutInMillis;
+ do {
+ if (Collections.disjoint(getSegmentsFor(tableNameWithType, false), segments)) {
+ return;
+ } else {
+ Thread.sleep(SEGMENT_CLEANUP_CHECK_INTERVAL_MS);
+ }
+ } while (System.currentTimeMillis() < endTimeMs);
+ throw new RuntimeException("Timeout while waiting for segments to be deleted");
Review Comment:
Also, suggest adding some details of the exception in Line 3176 to briefly explain why the request failed.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -3183,6 +3186,20 @@ public String startReplaceSegments(String tableNameWithType, List<String> segmen
return segmentLineageEntryId;
}
+ private void waitForSegmentsToDelete(String tableNameWithType, List<String> segments, long timeOutInMillis)
+ throws InterruptedException {
+ LOGGER.info("Wait segments to delete. timeout = {}ms, segments = {}", timeOutInMillis, segments);
+ long endTimeMs = System.currentTimeMillis() + timeOutInMillis;
+ do {
+ if (Collections.disjoint(getSegmentsFor(tableNameWithType, false), segments)) {
+ return;
+ } else {
+ Thread.sleep(SEGMENT_CLEANUP_CHECK_INTERVAL_MS);
+ }
+ } while (System.currentTimeMillis() < endTimeMs);
+ throw new RuntimeException("Timeout while waiting for segments to be deleted");
Review Comment:
log the table name and the timeout value here.
--
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] snleee commented on pull request #8838: Wait segment deletion in startReplaceSegment api to make sure there will be at most 2 data snapshots.
Posted by GitBox <gi...@apache.org>.
snleee commented on PR #8838:
URL: https://github.com/apache/pinot/pull/8838#issuecomment-1148922825
@jtao15 Can you add a bit more context to the description?
--
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