Skip to content

Conversation

@mxm
Copy link
Contributor

@mxm mxm commented Nov 4, 2016

This should fix the test failures in ReadSourceITCase caused by #1050.

@dhalperi Wouldn't it be nice to have the temporary folder prefix somewhere accessible as a static variable?

@dhalperi
Copy link
Contributor

dhalperi commented Nov 4, 2016

Max, I'm surely missing something. The temp location as a concept should be entirely internal to the FileBasedSink; the user, runner, and tests should only be checking the final name of the files.

Why does this code use the temp location at all?

@mxm
Copy link
Contributor Author

mxm commented Nov 4, 2016

As it stands now, the temp directory stays after the final results have been moved from it. The temp directory is in the same directory as the output path. Since the Flink test code checks for all files and directories in the output path, and directories not related to the results are regarded as invalid, we have to ignore the temp directory.

As long as the temp directory stays in the output path, we will have to provide some kind of constant to make tests aware of it. Unless, the directory not being deleted is actually a bug in FileBasedSink.

@dhalperi
Copy link
Contributor

dhalperi commented Nov 4, 2016

Directory not being deleted is a bug.

On Fri, Nov 4, 2016 at 11:51 AM, Max [email protected] wrote:

As it stands now, the temp directory stays after the final results have
been moved from it. The temp directory is in the same directory as the
output path. Since the Flink test code checks for all files and directories
in the output path, and directories not related to the results are regarded
as invalid, we have to ignore the temp directory.

As long as the temp directory stays in the output path, we will have to
provide some kind of constant to make tests aware of it. Unless, the
directory not being deleted is actually a bug.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1283 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAgIT0WwHUQklTcw5-cEWi-mz_E9F4Rlks5q636YgaJpZM4KpW8a
.

@mxm
Copy link
Contributor Author

mxm commented Nov 4, 2016

Alright, sorry for rushing. I thought it was actually a new desired behavior. Closing then until the issue is fixed.

@mxm mxm closed this Nov 4, 2016
@mxm
Copy link
Contributor Author

mxm commented Nov 4, 2016

Seems to get fixed here #1278.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants