Skip to content

Conversation

@ph
Copy link
Contributor

@ph ph commented Jan 18, 2017

The TemporaryFile#delete! method was raising an exception because we
were trying to close a file that was already close, since the
on_complete is executed in a thread nothing was present to catch and
log that specific error and the thread would just end up getting killed.

This PR add guards for closing an already close file, also add test
around the stale file uploader checker to make sure only 1 file is
remaining on disk after rotation.

For future debugging we also log any error that can occur in the
uploader callback.

Fixes: #122 #120

ph added 2 commits January 18, 2017 15:34
The TemporaryFile#delete! method was raising an exception because we
raryFile#delete! method was raising an exception because we
were trying to close a file that was already close, since the
on_complete is executed in a thread nothing was present to catch and
log that specific error and the thread would just end up getting killed.

This PR add guards for closing an already close file, also add test
around the stale file uploader checker to make sure only 1 file is
remaining on disk after roration.

For future debugging purpuse we also log any error that can occur in the
uploader callback.

Fixes: #122 #120
subject.register
subject.multi_receive_encoded(batch)
sleep(1) # the periodic check should have kick int
sleep(5) # the periodic check should have kick in
Copy link
Contributor Author

@ph ph Jan 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

give it a bit more time to do his stuff before doing any assert seems to reduce a lot of the noise.

Copy link

@suyograo suyograo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@elasticsearch-bot
Copy link

Pier-Hugues Pellerin merged this into the following branches!

Branch Commits
master 78c721d, 808609d

elasticsearch-bot pushed a commit that referenced this pull request Jan 18, 2017
@joshuaspence
Copy link
Contributor

Should we be using FileUtils::rm_r instead of FileUtils::rm_rf? It seems that the latter method ignores StandardError.

@ph
Copy link
Contributor Author

ph commented Jan 18, 2017

@joshuaspence This is a good point, I will make another PR with rm_r, I would prefer to be notified if something is wrong.

  • Wrong permission?
  • Trying to delete a non existing file?

@joshuaspence
Copy link
Contributor

Thanks!

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.

Temporary files are not deleted

4 participants