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
[FLINK-30812][yarn] Fix uploading local files when using YARN with S3 #21788
Conversation
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.
Please re-check to avoid Mockito per the Flink code contribution guidelines
flink-yarn/src/test/java/org/apache/flink/yarn/YarnApplicationFileUploaderTest.java
Outdated
Show resolved
Hide resolved
@gaborgsomogyi Can you take a look? |
I'm having vacation on the whole week w/o computer. Next week I can start w/ this... |
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.
Looks good, a minor found.
@@ -388,13 +388,20 @@ private Path copyToRemoteApplicationDir( | |||
(relativeDstPath.isEmpty() ? "" : relativeDstPath + "/") + localSrcPath.getName(); | |||
final Path dst = new Path(applicationDir, suffix); | |||
|
|||
final Path localSrcPathWithScheme; | |||
if (localSrcPath.toUri().getScheme() == null) { |
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.
Not sure about the internal details of the Uri parsing but isNullOrWhitespaceOnly
sounds more safe.
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.
Thank you, I agree, I've changed it
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, pending tests.
Rebased to fix failing CI tests |
@MartijnVisser are you fine w/ the PR? I'm merging if you say so... |
Waiting to end the feature freeze.... |
@flinkbot run azure |
Unless there are objections + azure is passing I'm intended to merge this tomorrow. |
What is the purpose of the change
Hadoop versions starting from 3.3.2 have reworked copying local files to remote using S3AFileSystem. Now it requires the passed local Hadoop Path to have a
scheme
specified.Brief change log
YarnApplicationFileUploader will append
file://
scheme for local path if none specifiedVerifying this change
I manually tested on a local single-node Hadoop 3.3.4 cluster with
fs.defaultFS
pointing to a local Minio S3 server. Creating a Flink YARN session failed before the fix, but worked after.Also added new unit test in
YarnApplicationFileUploaderTest
.Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no) noDocumentation