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/11/03 23:00:17 UTC
[GitHub] [pinot] npawar opened a new pull request, #9719: Remove leftover file before downloading segmentTar
npawar opened a new pull request, #9719:
URL: https://github.com/apache/pinot/pull/9719
Before this code change, if for some reason `segmentTarFile` already existed on the server (at `indexDir/tableName_REALTIME/segmentName.tar.gz`), then we would simply fail with FileAlreadyExistsException during `SegmentFetcherFactory.fetchSegmentToLocal(uri, segmentTarFile);`
The file could already exist in the scenario where server gets restarted or killed, before we get to the finally of this method, which is supposed to clean up the file.
```
private void downloadSegmentFromDeepStore(String segmentName, IndexLoadingConfig indexLoadingConfig, String uri) {
File segmentTarFile = new File(_indexDir, segmentName + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
try {
SegmentFetcherFactory.fetchSegmentToLocal(uri, segmentTarFile);
_logger.info("Downloaded file from {} to {}; Length of downloaded file: {}", uri, segmentTarFile,
segmentTarFile.length());
untarAndMoveSegment(segmentName, indexLoadingConfig, segmentTarFile);
} catch (Exception e) {
_logger.warn("Failed to download segment {} from deep store: ", segmentName, e);
throw new RuntimeException(e);
} finally {
FileUtils.deleteQuietly(segmentTarFile);
}
}
```
Cleaning up the file explicitly before fetching to local.
--
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] npawar merged pull request #9719: Remove leftover file before downloading segmentTar
Posted by GitBox <gi...@apache.org>.
npawar merged PR #9719:
URL: https://github.com/apache/pinot/pull/9719
--
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 #9719: Remove leftover file before downloading segmentTar
Posted by GitBox <gi...@apache.org>.
klsince commented on code in PR #9719:
URL: https://github.com/apache/pinot/pull/9719#discussion_r1013480928
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -468,6 +468,13 @@ void downloadAndReplaceSegment(String segmentName, SegmentZKMetadata segmentZKMe
private void downloadSegmentFromDeepStore(String segmentName, IndexLoadingConfig indexLoadingConfig, String uri) {
File segmentTarFile = new File(_indexDir, segmentName + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
Review Comment:
I see a similar method in BaseTableManager, which does this to prepare a temp root dir for downloading and uncompressing etc..
```
File tempRootDir = getTmpSegmentDataDir("tmp-" + segmentName + "-" + UUID.randomUUID());
```
In RealtimeTableDataManager, it actually also uses this util `getTmpSegmentDataDir` but inside `untarAndMoveSegment()` a few lines below. So maybe, we make those two places a bit more consistent by creating a tempRootDir.
But to make things robust, inside getTmpSegmentDataDir(), we do the check you added here to recreate tmpRootDir if it used to exist.
--
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 #9719: Remove leftover file before downloading segmentTar
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9719:
URL: https://github.com/apache/pinot/pull/9719#issuecomment-1302803999
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9719?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 [#9719](https://codecov.io/gh/apache/pinot/pull/9719?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (048f77d) into [master](https://codecov.io/gh/apache/pinot/commit/a5413962abe4ae6a24473243a48966115aa91f98?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a541396) will **decrease** coverage by `6.95%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #9719 +/- ##
============================================
- Coverage 69.91% 62.95% -6.96%
- Complexity 4855 5227 +372
============================================
Files 1950 1938 -12
Lines 104339 104136 -203
Branches 15804 15781 -23
============================================
- Hits 72945 65558 -7387
- Misses 26267 33714 +7447
+ Partials 5127 4864 -263
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `25.34% <0.00%> (+0.08%)` | :arrow_up: |
| integration2 | `24.56% <0.00%> (-0.03%)` | :arrow_down: |
| unittests1 | `67.51% <0.00%> (+0.09%)` | :arrow_up: |
| 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/9719?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ata/manager/realtime/RealtimeTableDataManager.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | `67.42% <0.00%> (-1.16%)` | :arrow_down: |
| [...pinot/controller/recommender/io/ConfigManager.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9Db25maWdNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/client/AggregationResultSet.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0FnZ3JlZ2F0aW9uUmVzdWx0U2V0LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ntroller/recommender/rules/impl/JsonIndexRule.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL0pzb25JbmRleFJ1bGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...routing/adaptiveserverselector/HybridSelector.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9hZGFwdGl2ZXNlcnZlcnNlbGVjdG9yL0h5YnJpZFNlbGVjdG9yLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...troller/recommender/io/metadata/FieldMetadata.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9tZXRhZGF0YS9GaWVsZE1ldGFkYXRhLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...outing/adaptiveserverselector/LatencySelector.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9hZGFwdGl2ZXNlcnZlcnNlbGVjdG9yL0xhdGVuY3lTZWxlY3Rvci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...roller/recommender/rules/impl/BloomFilterRule.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9ydWxlcy9pbXBsL0Jsb29tRmlsdGVyUnVsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...oller/api/resources/PinotControllerAppConfigs.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90Q29udHJvbGxlckFwcENvbmZpZ3MuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ler/recommender/data/generator/BytesGenerator.java](https://codecov.io/gh/apache/pinot/pull/9719/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-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9kYXRhL2dlbmVyYXRvci9CeXRlc0dlbmVyYXRvci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [286 more](https://codecov.io/gh/apache/pinot/pull/9719/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) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] npawar commented on a diff in pull request #9719: Remove leftover file before downloading segmentTar
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #9719:
URL: https://github.com/apache/pinot/pull/9719#discussion_r1013496785
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -468,6 +468,13 @@ void downloadAndReplaceSegment(String segmentName, SegmentZKMetadata segmentZKMe
private void downloadSegmentFromDeepStore(String segmentName, IndexLoadingConfig indexLoadingConfig, String uri) {
File segmentTarFile = new File(_indexDir, segmentName + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
Review Comment:
this code is very difficult to unit test, so for the sake of not creating any other side effects, I would prefer to leave it at this simple check.. I don't feel confident moving the segmentTarFile into a tmp dir and not breaking something else..
--
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] walterddr commented on a diff in pull request #9719: Remove leftover file before downloading segmentTar
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9719:
URL: https://github.com/apache/pinot/pull/9719#discussion_r1013475697
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -468,6 +468,13 @@ void downloadAndReplaceSegment(String segmentName, SegmentZKMetadata segmentZKMe
private void downloadSegmentFromDeepStore(String segmentName, IndexLoadingConfig indexLoadingConfig, String uri) {
File segmentTarFile = new File(_indexDir, segmentName + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
+ if (segmentTarFile.exists()) {
Review Comment:
should we make this part of `SegmentFetcherFactory.fetchSegmentToLocal(uri, segmentTarFile, overwrite=true);` ?
--
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] npawar commented on a diff in pull request #9719: Remove leftover file before downloading segmentTar
Posted by GitBox <gi...@apache.org>.
npawar commented on code in PR #9719:
URL: https://github.com/apache/pinot/pull/9719#discussion_r1013497194
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##########
@@ -468,6 +468,13 @@ void downloadAndReplaceSegment(String segmentName, SegmentZKMetadata segmentZKMe
private void downloadSegmentFromDeepStore(String segmentName, IndexLoadingConfig indexLoadingConfig, String uri) {
File segmentTarFile = new File(_indexDir, segmentName + TarGzCompressionUtils.TAR_GZ_FILE_EXTENSION);
+ if (segmentTarFile.exists()) {
Review Comment:
hmm, possible. but it's used in a bunch of places, and I don't want to make a call for what each method should do (maybe one of it is relying on the failure)
--
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