flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gabor Gevay (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (FLINK-2369) On Windows, in testFailingSortingDataSinkTask the temp file is not removed
Date Fri, 17 Jul 2015 11:26:04 GMT

    [ https://issues.apache.org/jira/browse/FLINK-2369?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14631211#comment-14631211
] 

Gabor Gevay commented on FLINK-2369:
------------------------------------

This is worse than I thought. The code is totally inconsistent about the relationship between
OutputFormat.close() and CleanupWhenUnsuccessful.tryCleanupOnError(). If you do a find usages
on tryCleanupOnError it gets called in four places in three different relations to close:
1. DataSinkTask.cancel(): After close
2. Exception handler in DataSinkTask.invoke: Before close
3. FileSinkFunction.close: When catching an exception thrown by close
4. FileSinkFunction.flush: This is before close in FileSinkFunction.close.

I see two approaches to fixing this:

A. Fix all of the above to call tryCleanupOnError only after close.
- 2. is easy: just introduce a flag shouldCallTryCleanup, and set it to true instead where
now tryCleanupOnError is called, and do the actual tryCleanupOnError call after the close
in the finally block if the flag is set.
- You can't really do anything in case of 3., because things must be really messed up if we
get to that catch block. I mean, the close already failed here, so something very bad must
be going on.
- I am not sure about 4: Does the same approach of introducing a flag and attending to it
after close would work? FileSinkFunction.close would be a natural place to attend to the flag,
but I am not even sure whether it gets called in case of an error.

B. Modify the contract of tryCleanupOnError to not give any guarantees about the relation
to close.
This means that it should do a close on its own if neccessary. This would basically mean,
that the implementor of the interface got all the burden of making sure that it can handle
all of the above 4 cases. What makes this unpleasant is that the javadoc of close says that
"When this method is called, the output format it guaranteed to be opened.", which means that
some subclass might get "surprised" if we just call close from FileOutputFormat.tryCleanupOnError
and it ends up called twice.

> On Windows, in testFailingSortingDataSinkTask the temp file is not removed
> --------------------------------------------------------------------------
>
>                 Key: FLINK-2369
>                 URL: https://issues.apache.org/jira/browse/FLINK-2369
>             Project: Flink
>          Issue Type: Bug
>          Components: Distributed Runtime
>         Environment: Windows 7 64-bit, JDK 8u51
>            Reporter: Gabor Gevay
>            Assignee: Gabor Gevay
>            Priority: Minor
>
> The test fails with the assert at the very end ("Temp output file has not been removed").
This happens because FileOutputFormat.tryCleanupOnError can't delete the file, because it
is still open (note, that Linux happily deletes open files).
> A fix would be to have the  this.format.close();  not just in the finally block (in DataSinkTask.invoke),
but also before the tryCleanupOnError call (around line 217).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message