Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] correct the dir for building segments in FileIngestionHelper #9591

Merged

Conversation

zhtaoxiang
Copy link
Contributor

@zhtaoxiang zhtaoxiang commented Oct 13, 2022

  1. Currently, FileIngestionHelper uses controller data dir for building segments. The working dir ends up to be /<folder-to-start-pinot>/<controller.data.dir>/upload_dir/working_dir_<table-name>_<ts>, which is not expected. There may be permission issue when using this dir and we may get java.lang.IllegalStateException: Could not create directory for downloading input file locally: ... Ideally, the working dir should be /<controller.local.temp.dir>/ingestion_dir/working_dir_<table-name>_<ts>.
  2. To avoid using the same working directory when multiple tasks are running in parallel, append a random string to the working dir name.

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. can we add a test in PinotIngestionRestletResourceStatelessTest?

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #9591 (caf21f9) into master (9d16896) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             master    #9591    +/-   ##
==========================================
  Coverage     70.01%   70.02%            
- Complexity     4842     4871    +29     
==========================================
  Files          1934     1936     +2     
  Lines        103312   103475   +163     
  Branches      15695    15714    +19     
==========================================
+ Hits          72336    72455   +119     
- Misses        25873    25923    +50     
+ Partials       5103     5097     -6     
Flag Coverage Δ
integration1 25.94% <0.00%> (+<0.01%) ⬆️
integration2 24.74% <0.00%> (+0.11%) ⬆️
unittests1 67.42% <ø> (+0.04%) ⬆️
unittests2 15.60% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...r/api/resources/PinotIngestionRestletResource.java 55.26% <100.00%> (ø)
...che/pinot/controller/util/FileIngestionHelper.java 91.25% <100.00%> (+0.11%) ⬆️
...ntroller/helix/core/minion/TaskMetricsEmitter.java 34.88% <0.00%> (-51.17%) ⬇️
...nt/local/loader/DefaultSegmentDirectoryLoader.java 61.53% <0.00%> (-38.47%) ⬇️
.../filter/predicate/InPredicateEvaluatorFactory.java 74.43% <0.00%> (-4.52%) ⬇️
...ion/function/DistinctCountAggregationFunction.java 50.22% <0.00%> (-4.04%) ⬇️
...r/validation/RealtimeSegmentValidationManager.java 74.32% <0.00%> (-2.71%) ⬇️
...core/query/pruner/SelectionQuerySegmentPruner.java 83.52% <0.00%> (-2.36%) ⬇️
...lix/core/minion/PinotHelixTaskResourceManager.java 40.70% <0.00%> (-2.27%) ⬇️
.../org/apache/pinot/core/startree/StarTreeUtils.java 76.28% <0.00%> (-2.07%) ⬇️
... and 37 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zhtaoxiang
Copy link
Contributor Author

lgtm. can we add a test in PinotIngestionRestletResourceStatelessTest?

okay, will do

@zhtaoxiang zhtaoxiang changed the title [bugfix] use local temp dir for building segments in FileIngestionHelper [bugfix] correct the dir for building segments in FileIngestionHelper Oct 14, 2022
@zhtaoxiang
Copy link
Contributor Author

lgtm. can we add a test in PinotIngestionRestletResourceStatelessTest?

added a simple check, please take a look to see if it's okay or not.

@walterddr walterddr merged commit 4caab2b into apache:master Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants