You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "swaminathanmanish (via GitHub)" <gi...@apache.org> on 2024/02/01 18:48:33 UTC
[PR] Add segment metadata deletion code to segment deletion path [pinot]
swaminathanmanish opened a new pull request, #12350:
URL: https://github.com/apache/pinot/pull/12350
**Whats in the PR:**
Added segment metadata deletion from remote store, in the segment deletion path.
**Why its needed:**
Through [PR](https://github.com/apache/pinot/pull/10034), we got the ability to upload segment metadata to remote store, which removes the need to download the entire segment to parse metadata, while creating segment metadata in Zk (metadata push in controller). This change here is to ensure that we always clean up segment metadata (catch all deletion), when the associated segment gets deleted from remote store. Segment and segment metadata are stored in the same remote directory.
--
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] Add segment metadata deletion code to segment deletion path [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12350:
URL: https://github.com/apache/pinot/pull/12350#issuecomment-1922068621
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12350?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `12 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`6cc1915`)](https://app.codecov.io/gh/apache/pinot/commit/6cc19151404cfc7acf5677ef3395e129a978e2d8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.65% compared to head [(`a98dc08`)](https://app.codecov.io/gh/apache/pinot/pull/12350?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 0.00%.
> Report is 10 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../controller/helix/core/SegmentDeletionManager.java](https://app.codecov.io/gh/apache/pinot/pull/12350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1NlZ21lbnREZWxldGlvbk1hbmFnZXIuamF2YQ==) | 0.00% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12350?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12350 +/- ##
=============================================
- Coverage 61.65% 0.00% -61.66%
=============================================
Files 2421 2348 -73
Lines 131872 128763 -3109
Branches 20346 19922 -424
=============================================
- Hits 81308 0 -81308
- Misses 44600 128763 +84163
+ Partials 5964 0 -5964
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12350/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/12350/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/12350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12350/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/12350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12350/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/12350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.54%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.65%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12350/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/12350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.66%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12350/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12350/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/12350?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).
--
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] Add segment metadata deletion code to segment deletion path [pinot]
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #12350:
URL: https://github.com/apache/pinot/pull/12350#discussion_r1475077573
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java:
##########
@@ -357,6 +358,9 @@ public void createTableAndSegmentFiles(File tempDir, List<String> segmentIds)
tableDir.mkdir();
for (String segmentId : segmentIds) {
createTestFileWithAge(tableDir.getAbsolutePath() + File.separator + segmentId, 0);
+ // Create segment metadata file
+ createTestFileWithAge(
Review Comment:
Is this covering the test for metadata delete?
--
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] Add segment metadata deletion code to segment deletion path [pinot]
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee merged PR #12350:
URL: https://github.com/apache/pinot/pull/12350
--
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] Add segment metadata deletion code to segment deletion path [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on PR #12350:
URL: https://github.com/apache/pinot/pull/12350#issuecomment-1921999009
@snleee - Could you take a look? Thanks !
--
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] Add segment metadata deletion code to segment deletion path [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #12350:
URL: https://github.com/apache/pinot/pull/12350#discussion_r1475083432
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/util/SegmentDeletionManagerTest.java:
##########
@@ -357,6 +358,9 @@ public void createTableAndSegmentFiles(File tempDir, List<String> segmentIds)
tableDir.mkdir();
for (String segmentId : segmentIds) {
createTestFileWithAge(tableDir.getAbsolutePath() + File.separator + segmentId, 0);
+ // Create segment metadata file
+ createTestFileWithAge(
Review Comment:
yes added metadata file as well. At the end of the test, there's a verification that the directory does not have any files -
```
try {
Assert.assertEquals(tableDir.listFiles().length, 0);
Assert.assertTrue(!deletedTableDir.exists() || deletedTableDir.listFiles().length == );
```
--
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