You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "tibrewalpratik17 (via GitHub)" <gi...@apache.org> on 2024/09/09 20:27:32 UTC
[PR] Move untar method to BaseTaskExecutor and untar with peerDownload segment if deepstore corrupted [pinot]
tibrewalpratik17 opened a new pull request, #13964:
URL: https://github.com/apache/pinot/pull/13964
(no comment)
--
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] Move untar method to BaseTaskExecutor and untar with peerDownload segment if deepstore corrupted [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13964:
URL: https://github.com/apache/pinot/pull/13964#discussion_r1752876484
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java:
##########
@@ -200,30 +200,24 @@ public List<SegmentConversionResult> executeTask(PinotTaskConfig pinotTaskConfig
for (int i = 0; i < downloadURLs.length; i++) {
String segmentName = segmentNames[i];
- // Download the segment file
+ // Download and decompress the segment file
_eventObserver.notifyProgress(_pinotTaskConfig,
- String.format("Downloading segment from: %s (%d out of %d)", downloadURLs[i], (i + 1),
+ String.format("Downloading and decompressing segment from: %s (%d out of %d)", downloadURLs[i], (i + 1),
downloadURLs.length));
- File tarredSegmentFile = new File(tempDataDir, "tarredSegmentFile_" + i);
+ File indexDir;
try {
- downloadSegmentToLocal(tableNameWithType, segmentName, downloadURLs[i], taskType, tarredSegmentFile);
+ indexDir = downloadSegmentToLocalAndUntar(tableNameWithType, segmentName, downloadURLs[i], taskType,
+ tempDataDir, "_" + i);
+ if (indexDir == null) {
Review Comment:
Makes sense. Updated.
--
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] Move untar method to BaseTaskExecutor and untar with peerDownload segment if deepstore corrupted [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13964:
URL: https://github.com/apache/pinot/pull/13964#issuecomment-2339100551
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13964?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `16 lines` in your changes missing coverage. Please review.
> Project coverage is 0.00%. 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 [(`dab4797`)](https://app.codecov.io/gh/apache/pinot/commit/dab4797d14f08ee3facd81794d64246860ae9383?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1010 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/13964?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...he/pinot/plugin/minion/tasks/BaseTaskExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/13964?src=pr&el=tree&filepath=pinot-plugins%2Fpinot-minion-tasks%2Fpinot-minion-builtin-tasks%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fplugin%2Fminion%2Ftasks%2FBaseTaskExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZVRhc2tFeGVjdXRvci5qYXZh) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13964?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../tasks/BaseMultipleSegmentsConversionExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/13964?src=pr&el=tree&filepath=pinot-plugins%2Fpinot-minion-tasks%2Fpinot-minion-builtin-tasks%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fplugin%2Fminion%2Ftasks%2FBaseMultipleSegmentsConversionExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZU11bHRpcGxlU2VnbWVudHNDb252ZXJzaW9uRXhlY3V0b3IuamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13964?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ion/tasks/BaseSingleSegmentConversionExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/13964?src=pr&el=tree&filepath=pinot-plugins%2Fpinot-minion-tasks%2Fpinot-minion-builtin-tasks%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fplugin%2Fminion%2Ftasks%2FBaseSingleSegmentConversionExecutor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvQmFzZVNpbmdsZVNlZ21lbnRDb252ZXJzaW9uRXhlY3V0b3IuamF2YQ==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13964?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (dab4797). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (dab4797) |
>|------|------|------|
>|integration|7|1|
>|integration2|3|1|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|0|
>|unittests1|2|0|
>|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 #13964 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2514 +78
Lines 133233 139015 +5782
Branches 20636 21370 +734
=============================================
- Hits 82274 0 -82274
- Misses 44911 139015 +94104
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13964/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/13964/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/13964/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/13964/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/13964/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/13964/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/13964/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.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13964/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.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13964/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/13964/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.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13964/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/13964/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/13964/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/13964?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).
--
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] Move untar method to BaseTaskExecutor and untar with peerDownload segment if deepstore corrupted [pinot]
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana merged PR #13964:
URL: https://github.com/apache/pinot/pull/13964
--
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] Move untar method to BaseTaskExecutor and untar with peerDownload segment if deepstore corrupted [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13964:
URL: https://github.com/apache/pinot/pull/13964#discussion_r1752904140
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java:
##########
@@ -132,9 +140,19 @@ protected void downloadSegmentToLocal(String tableNameWithType, String segmentNa
Collections.shuffle(uris);
return uris;
}, tarredSegmentFile, crypterName);
+ // untar the segment file
+ indexDir = TarCompressionUtils.untar(tarredSegmentFile, segmentDir).get(0);
} else {
throw e;
}
+ } finally {
+ if (!FileUtils.deleteQuietly(tarredSegmentFile)) {
+ LOGGER.warn("Failed to delete tarred input segment: {}", tarredSegmentFile.getAbsolutePath());
+ }
+ }
+ if (indexDir == null) {
Review Comment:
Will it ever be `null`? Seems like it can never be `null`, or exception is thrown
--
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] Move untar method to BaseTaskExecutor and untar with peerDownload segment if deepstore corrupted [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13964:
URL: https://github.com/apache/pinot/pull/13964#discussion_r1752771129
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseMultipleSegmentsConversionExecutor.java:
##########
@@ -200,30 +200,24 @@ public List<SegmentConversionResult> executeTask(PinotTaskConfig pinotTaskConfig
for (int i = 0; i < downloadURLs.length; i++) {
String segmentName = segmentNames[i];
- // Download the segment file
+ // Download and decompress the segment file
_eventObserver.notifyProgress(_pinotTaskConfig,
- String.format("Downloading segment from: %s (%d out of %d)", downloadURLs[i], (i + 1),
+ String.format("Downloading and decompressing segment from: %s (%d out of %d)", downloadURLs[i], (i + 1),
downloadURLs.length));
- File tarredSegmentFile = new File(tempDataDir, "tarredSegmentFile_" + i);
+ File indexDir;
try {
- downloadSegmentToLocal(tableNameWithType, segmentName, downloadURLs[i], taskType, tarredSegmentFile);
+ indexDir = downloadSegmentToLocalAndUntar(tableNameWithType, segmentName, downloadURLs[i], taskType,
+ tempDataDir, "_" + i);
+ if (indexDir == null) {
Review Comment:
Suggest directly let `downloadSegmentToLocalAndUntar()` throw exception instead of relying on `null` to pass the error. It will be very hard to debug when it returns `null` and we catch the exception with only `Unable to download and unter ...` in the error message
--
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] Move untar method to BaseTaskExecutor and untar with peerDownload segment if deepstore corrupted [pinot]
Posted by "tibrewalpratik17 (via GitHub)" <gi...@apache.org>.
tibrewalpratik17 commented on code in PR #13964:
URL: https://github.com/apache/pinot/pull/13964#discussion_r1752917639
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseTaskExecutor.java:
##########
@@ -132,9 +140,19 @@ protected void downloadSegmentToLocal(String tableNameWithType, String segmentNa
Collections.shuffle(uris);
return uris;
}, tarredSegmentFile, crypterName);
+ // untar the segment file
+ indexDir = TarCompressionUtils.untar(tarredSegmentFile, segmentDir).get(0);
} else {
throw e;
}
+ } finally {
+ if (!FileUtils.deleteQuietly(tarredSegmentFile)) {
+ LOGGER.warn("Failed to delete tarred input segment: {}", tarredSegmentFile.getAbsolutePath());
+ }
+ }
+ if (indexDir == null) {
Review Comment:
Hmm good point! I see in almost all scenarios exception been thrown or indexDir been non-null. Updated!
--
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