Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filter out non-clacier or clacier which already restored #828

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wangzhihao
Copy link

@wangzhihao wangzhihao commented Jan 22, 2017

Filter out non-GLACIER files and GLACIER files which already start or finish restored in s3cmd restore

@fviard
Copy link
Contributor

fviard commented Jan 22, 2017

Thank you for your contribution.
The idea is interesting, but isn't it more efficient to simply still send only the "restore" request and ignore based on the error/reply? Maybe the "output" message could be fixed to only say "restore" for files that will in effect be restored.
In the python way "ask forgiveness, not permission".
As in your PR, you suggest to issue a "get_info" first for each file.

@wangzhihao
Copy link
Author

wangzhihao commented Jan 23, 2017

Yeah, error-and-reply sounds more graceful. But my main concern is that the error code InvalidObjectState is not clear to indicate non-glacier. Is it safe for us to ignore all InvalidObjectState error?

   if e.code == "InvalidObjectState":
     warning("%s: %s" % (e.message, item['object_uri_str']))

Secondary, to extend this idea, we might let each file to be independent to the others. I.E. We process all files and don't let any error stop the iteration. Do you think is this option safe?

@fviard
Copy link
Contributor

fviard commented Oct 11, 2022

Sorry that I forgot to reply since so long, i don't know if you are still looking at this MR, but the reply is yes, skipping all InvalidObjectState looks ok to me.
We can probably have a more explicit message prefix in the warning also ("Skipping as not a Glacier object or not in a restorable state:")

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