Skip to content

Conversation

@jessewiles
Copy link
Contributor

@jessewiles jessewiles commented Sep 20, 2017

Proposed fix for: #187

This fix has a couple of parts. First, it gets rid of the get_key call, which is not necessary for setting the ACL. In our tests, an exception in this get_key call caused the entire upload to fail, which is probably NOT the desired result.

Secondly, this fix uses the set_acl directly on the S3 Bucket object (http://boto.cloudhackers.com/en/latest/ref/s3.html#boto.s3.bucket.Bucket.set_acl). This allows us to avoid the problematic get_key call.

Finally, the exception was blank in our testing. This fix adds an explicit call to traceback to help clarify the cause of the error.

@timvaillancourt
Copy link
Contributor

Hi @jessewiles,

Thanks for your fixes to the S3 Uploader! We will be freezing the code for the next 7 days or so to stabilise the code for the 1.2.0 release during our Percona Live Europe Conference. I'll review this code after that freeze.

@jessewiles
Copy link
Contributor Author

jessewiles commented Sep 22, 2017

Thanks Tim! For reference, here's the log which provides context for this particular fix. Cheers.

https://gist.github.com/jessewiles/79da07545a4dffae96c9d986f3bdb6ae

  • Notice this section to see the upload complete successfully, but the subsequent HEAD call to get the key triggers failure cleanup:

https://gist.github.com/jessewiles/79da07545a4dffae96c9d986f3bdb6ae#file-mongodb_consistent_backup-log-L300

Copy link
Contributor

@timvaillancourt timvaillancourt left a comment

Choose a reason for hiding this comment

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

Is it possible to use logging.exception() here instead of importing+using the traceback module? I believe this will arrive at the same result.

https://docs.python.org/3/library/logging.html#logging.Logger.exception

self.bucket.set_acl(self.s3_acl, key_name)
except Exception as ex:
logging.warn("Unable to set ACLs on uploaded key. Exception: {}".format(str(ex)))
import traceback
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use logging.exception() here instead of importing+using the traceback module? I believe this will arrive at the same result.

https://docs.python.org/3/library/logging.html#logging.Logger.exception

@jessewiles
Copy link
Contributor Author

Done.

@timvaillancourt timvaillancourt merged commit 05343da into Percona-Lab:master Dec 1, 2017
@timvaillancourt
Copy link
Contributor

Thanks @jessewiles!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants