Mailing-List: contact issues-help@flink.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@flink.apache.org
Date: Fri, 17 Jul 2015 11:26:04 +0000 (UTC)
From: "Gabor Gevay (JIRA)" <jira@apache.org>
To: issues@flink.apache.org
Message-ID: <JIRA.12845634.1437061384000.214558.1437132364528@Atlassian.JIRA>
In-Reply-To: <JIRA.12845634.1437061384000@Atlassian.JIRA>
References: <JIRA.12845634.1437061384000@Atlassian.JIRA>
 <JIRA.12845634.1437061384456@arcas>
Subject: [jira] [Commented] (FLINK-2369) On Windows, in
 testFailingSortingDataSinkTask the temp file is not removed
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ 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)