-
Notifications
You must be signed in to change notification settings - Fork 391
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
changed to look specifically for part files rather than listing the files and searching through them #505
Conversation
Is it faster? What if another compression technique is being used, i.e. 7Zip, then we would have |
Codecov Report
@@ Coverage Diff @@
## master #505 +/- ##
==========================================
- Coverage 86.74% 86.03% -0.71%
==========================================
Files 347 347
Lines 11859 11860 +1
Branches 388 607 +219
==========================================
- Hits 10287 10204 -83
- Misses 1572 1656 +84
Continue to review full report at Codecov.
|
} | ||
val finalPath = partPath.getOrElse(path) | ||
|
||
val partFile = new Path(pathString, "part-00000") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better configure the part file name on the write call path so we don't have to guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will always be this but the compression codec will determine the extension
|
||
val partFile = new Path(pathString, "part-00000") | ||
val partZipped = new Path(pathString, "part-00000.gz") | ||
val finalPath = if (fs.exists(partZipped)) partZipped else if (fs.exists(partFile)) partFile else path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exists add redundant filesystem calls that will be even more expensive with S3
we can just call open directly and catch FileNotFound to try another filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gerashegalov fixed to just try loading
@gerashegalov suggested this for faster reading |
@tovbinm it should be faster on object stores since you have fast path lookups but slow entry list performance. |
@tovbinm this covers no compression and our default settings as well as passing in the exact full path. Do you think we need to cover more possibilities? |
…rifAI into lm/loadSpecificFiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor comment
} | ||
} | ||
} match { | ||
case Failure(e) => throw new RuntimeException(s"Failed to load workflow because of $e") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let us keep e
as the cause exception as well, i.e. provided as the second parameter to the RTE constructor.
I don't think we need to make it that complex. At most I would make Compression Codec configurable instead of doing any guesses at all. |
Related issues
Refer to issue(s) addressed in this pull request from Issues page.
Describe the proposed solution
A clear and concise description of what the changes are.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the changes here.