Skip to content

Conversation

@ihorkhavkin
Copy link
Contributor

Fixes issue with upload stopping when move_file_to_bucket fails and cleans up shutdown a bit (as case doesn't seem to work on class comparison - http://stackoverflow.com/a/5694333/5657104 )

@ihorkhavkin
Copy link
Contributor Author

#79
Will sign CLA after my employer's approval.

@ihorkhavkin
Copy link
Contributor Author

ihorkhavkin commented May 24, 2016

CLA was signed, please let me know if something else needs to be done for CLA check to pass

@andrewvc
Copy link
Contributor

andrewvc commented Aug 5, 2016

@ihorkhavkin thanks for the PR. Apologies for the extreme delay in looking at this, I'm taking a look now :)

@andrewvc andrewvc self-assigned this Aug 5, 2016
@andrewvc
Copy link
Contributor

andrewvc commented Aug 5, 2016

This mostly looks good to merge to me, but I want to hold off for a minute. It looks to me like nothing properly terminates the worker threads on shutdown. That seems problematic.

The tests all pass but I want to do a manual test to see how that goes. I'm not sure if this is the PR to fix this in (probably not) but I want to think about it over the weekend.

rescue Exception => ex
@logger.error("failed to upload, will re-enqueue #{file} for upload",
:ex => ex, :backtrace => ex.backtrace)
unless file.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

The file cannot be nil, since we are dequeuing stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, It could be nil when queue #deq get interrupted at shutdown.

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussing with @andrewvc we don't think this case happen in the JVM.

@andrewvc andrewvc mentioned this pull request Aug 8, 2016
@andrewvc
Copy link
Contributor

andrewvc commented Aug 8, 2016

Merged in #90

@andrewvc andrewvc closed this Aug 8, 2016
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.

3 participants