-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-26957][runtime] Removes flush in FileSystemJobResultStore.createDirtyResultInternal #19304
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
Conversation
144146f to
df4a61b
Compare
|
@flinkbot run azure |
|
I forgot to change the unit tests expected error from |
|
I added a test case for closing the OutputStream twice in the most-recent force-push. |
dmvk
left a comment
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.
Hi @XComp, this already looks really good. Great job! <3 I've added few comments and questions, please take a look.
flink-core/src/main/java/org/apache/flink/core/fs/local/LocalDataOutputStream.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/core/fs/local/LocalDataOutputStream.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/core/fs/local/LocalDataOutputStream.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/core/fs/local/LocalDataOutputStream.java
Outdated
Show resolved
Hide resolved
...untime/src/main/java/org/apache/flink/runtime/highavailability/FileSystemJobResultStore.java
Outdated
Show resolved
Hide resolved
|
CI looks good - I created a 1.15 backport already to make CI pass (hoping that there are no additional changes required) |
dmvk
left a comment
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, great job! 👍 Can you please fix the commit history before merging?
flink-core/src/test/java/org/apache/flink/core/fs/FileSystemBehaviorTestSuite.java
Outdated
Show resolved
Hide resolved
The writeValue calls close by default internally. Calling flush afterwards could cause errors. It's also not really necessary. OutputStream.flush does not guarantee persistence according to its JavaDoc. In contrast, calling close does guarantee it.
…ify that no operation is allowed on a closed stream
|
I'm gonna merge. I only dropped on commit. |
The
writeValuecalls close by default internally. Calling flush afterwardscould cause errors. It's also not really necessary. OutputStream.flush does
not guarantee persistence according to its JavaDoc. In contrast, calling
close does guarantee it.
What is the purpose of the change
Some FileSystem implementations fail when calling flush on a closed OutputStream (e.g. HDFS).
To cover this behavior in the tests, we added this check to the
LocalDataOutputStream.Brief change log
OutputStream.closecall as stated in the JavaDocLocalDataOutputStreamto cover this behavior in tests.Verifying this change
LocalFileSystemTestto check for the assertionsDoes this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation