Issue 7311079: Adding retry logic to setmeta's optimistic concurrency approach.
|
Created:
11 years, 2 months ago by zwilt Modified:
11 years, 2 months ago CC:
bigstore-team_google.com Base URL:
http://gsutil.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdding retry logic to setmeta's optimistic concurrency approach.
Depends on (soon to be submitted for review) change to boto to make s3's copy_key pass headers correctly. Without the boto change, this CL will have no effect on correctness.
My current plan is to abstract the retry logic after this and https://codereview.appspot.com/7235060 get pushed.
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fixed bad Python practices from last patch, as per jterrace@'s and mfschwartz@'s comments #
Total comments: 4
Patch Set 3 : setmeta now raises an error after all retries are exhausted. #MessagesTotal messages: 16
I'm going to work on getting boto tests/authentication up and running tonight so that I can submit the pull request necessary for this to work. Luckily, the boto change is very trivial.
Sign in to reply to this message.
Emailing correct reviewers...
Sign in to reply to this message.
https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:225: meta_generation = key.meta_generation exp_src_uri should have .generation and .meta_generation properties. Can you try using that instead? We like to avoid using the key directly when possible. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:243: self.THREADED_LOGGER.warn('Collision - %d tries left.' % retry) The logger functions allow you to do: .warn('Collision - %d tries left.', retry) instead. It's a little safer as it masks exceptions when possible. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:247: raise response_error You can just use "raise" here. It will raise the last exception.
Sign in to reply to this message.
FLight is about to end so sending the comments I was able to write so far. More in next review rev. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:219: def _SetMetadataFunc(name_expansion_result, retry=3): How does this compare with Benson's retry handling in chacl? It would be good if they worked uniformly. In partic, maybe we should add a config variable for num_concur_control_retries, and use it from both commands. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:221: name_expansion_result.GetExpandedUriStr()) This way of building the URI will never have generation and meta_generation, since I believe you're expanding from a version-less URI string. I think we need to add a param to NameExpansionIterator to make it return BLR's with version-specific details. And then once you do that you should no longer have the "if generation" conditionals below - just always rely on having the versioning info. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:222: self.THREADED_LOGGER.info('Setting metadata on %s...' % exp_src_uri) Might be good to print (retry X) for attempts past the first one. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:225: meta_generation = key.meta_generation On 2013/02/12 17:44:02, jterrace wrote: > exp_src_uri should have .generation and .meta_generation properties. Can you try > using that instead? We like to avoid using the key directly when possible. +1 Further detail about why: Eventually we want to make Apiary the primary lib beneath gsutil, with boto supporting just the legacy (S3-compat) API. To do that, we need StorageUri to be *the* abstraction layer. The gsutil code has other places that currently directly interact with the underlying boto objects, but we're trying to minimize how many new such cases we introduce.
Sign in to reply to this message.
Updated to fix bad Python practices - please see comments below about using boto keys for hackery and abstracting retry logic. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:219: def _SetMetadataFunc(name_expansion_result, retry=3): On 2013/02/12 18:02:43, Mike Schwartz wrote: > How does this compare with Benson's retry handling in chacl? > It would be good if they worked uniformly. > In partic, maybe we should add a config variable for num_concur_control_retries, > and use it from both commands. I pulled this number from the most recent chacl CL I could find. I would like to abstract as much as possible of the retry logic, but I was planning to do it in a separate CL after Benson's code is pushed. Would you prefer that I go ahead and do it with this CL? https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:221: name_expansion_result.GetExpandedUriStr()) On 2013/02/12 18:02:43, Mike Schwartz wrote: > This way of building the URI will never have generation and meta_generation, > since I believe you're expanding from a version-less URI string. I think we need > to add a param to NameExpansionIterator to make it return BLR's with > version-specific details. And then once you do that you should no longer have > the "if generation" conditionals below - just always rely on having the > versioning info. As you said, exp_src_uri seems to only have version info if it does an actual expansion (like gs://bucket/*) - if you specify a particular file with no wildcards, it doesn't have version info. I was getting around this by using the get_key() method (basically with ugly hackery, as you and Jeff noted below). If I'm interpreting your comment correctly, the NameExpansionIterator does not currently have an option to provide this - is it something you'd like me to add with this CL? https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:222: self.THREADED_LOGGER.info('Setting metadata on %s...' % exp_src_uri) On 2013/02/12 18:02:43, Mike Schwartz wrote: > Might be good to print > (retry X) > for attempts past the first one. I've currently got it printing a number of remaining tries (line 243) after it detects a collision (although, the wording there should probably be more descriptive). Would you rather it be printed at 243, here, or both? https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:225: meta_generation = key.meta_generation On 2013/02/12 18:02:43, Mike Schwartz wrote: > On 2013/02/12 17:44:02, jterrace wrote: > > exp_src_uri should have .generation and .meta_generation properties. Can you > try > > using that instead? We like to avoid using the key directly when possible. > > +1 > Further detail about why: Eventually we want to make Apiary the primary lib > beneath gsutil, with boto supporting just the legacy (S3-compat) API. To do > that, we need StorageUri to be *the* abstraction layer. The gsutil code has > other places that currently directly interact with the underlying boto objects, > but we're trying to minimize how many new such cases we introduce. I'm currently relying on this to get version info. See response to the comment about building the URI above. It does make sense, though, to try to avoid this, if it all possible. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:243: self.THREADED_LOGGER.warn('Collision - %d tries left.' % retry) On 2013/02/12 17:44:02, jterrace wrote: > The logger functions allow you to do: > .warn('Collision - %d tries left.', retry) > instead. It's a little safer as it masks exceptions when possible. I didn't realize that (I blindly copied syntax from somewhere) - it's now fixed. Thanks for pointing out the difference. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:244: time.sleep(random.uniform(0.5, 1.0)) I also set this backoff strategy to be consistent with Benson's chacl. This is something else I'd like to abstract out. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:247: raise response_error On 2013/02/12 17:44:02, jterrace wrote: > You can just use "raise" here. It will raise the last exception. I've fixed this as well, thanks.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:219: def _SetMetadataFunc(name_expansion_result, retry=3): On 2013/02/12 21:50:44, zwilt wrote: > On 2013/02/12 18:02:43, Mike Schwartz wrote: > > How does this compare with Benson's retry handling in chacl? > > It would be good if they worked uniformly. > > In partic, maybe we should add a config variable for > num_concur_control_retries, > > and use it from both commands. > > I pulled this number from the most recent chacl CL I could find. I would like to > abstract as much as possible of the retry logic, but I was planning to do it in > a separate CL after Benson's code is pushed. Would you prefer that I go ahead > and do it with this CL? Doing it in a separate CL sounds right. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:221: name_expansion_result.GetExpandedUriStr()) On 2013/02/12 21:50:44, zwilt wrote: > On 2013/02/12 18:02:43, Mike Schwartz wrote: > > This way of building the URI will never have generation and meta_generation, > > since I believe you're expanding from a version-less URI string. I think we > need > > to add a param to NameExpansionIterator to make it return BLR's with > > version-specific details. And then once you do that you should no longer have > > the "if generation" conditionals below - just always rely on having the > > versioning info. > > As you said, exp_src_uri seems to only have version info if it does an actual > expansion (like gs://bucket/*) - if you specify a particular file with no > wildcards, it doesn't have version info. > > I was getting around this by using the get_key() method (basically with ugly > hackery, as you and Jeff noted below). If I'm interpreting your comment > correctly, the NameExpansionIterator does not currently have an option to > provide this - is it something you'd like me to add with this CL? Thinking about it some more, let's leave it the way you have it. NameExpansionIterator is a pretty complex beast, and I'm not sure we want to introduce versioning complexities there. Maybe we could refactor that in the future; for now, what you have seems fine. https://codereview.appspot.com/7311079/diff/1/src/gslib/commands/setmeta.py#n... src/gslib/commands/setmeta.py:222: self.THREADED_LOGGER.info('Setting metadata on %s...' % exp_src_uri) On 2013/02/12 21:50:44, zwilt wrote: > On 2013/02/12 18:02:43, Mike Schwartz wrote: > > Might be good to print > > (retry X) > > for attempts past the first one. > > I've currently got it printing a number of remaining tries (line 243) after it > detects a collision (although, the wording there should probably be more > descriptive). Would you rather it be printed at 243, here, or both? I missed this before. What you have there is fine.
Sign in to reply to this message.
https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.p... src/gslib/commands/setmeta.py:242: return Are we hiding error information here by returning instead of raising once we give up?
Sign in to reply to this message.
https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.p... src/gslib/commands/setmeta.py:242: return On 2013/02/12 22:16:17, thobrla wrote: > Are we hiding error information here by returning instead of raising once we > give up? I'm not sure how much we're hiding, since the retry process is logged. I think it's a question of whether a single setmeta call that fails should throw an error, blocking all the others (in the case of wildcards, etc., unless this is prevented elsewhere already) or should fail with a warning, which could be more easily missed. I'm not sure what the right answer is based on gsutil's current architecture, but I could see it going either way. If there's something in place to prevent one failed update from breaking others that were part of the same call, my vote would definitely be for making this an error.
Sign in to reply to this message.
https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.p... src/gslib/commands/setmeta.py:242: return On 2013/02/12 22:32:14, zwilt wrote: > On 2013/02/12 22:16:17, thobrla wrote: > > Are we hiding error information here by returning instead of raising once we > > give up? > > I'm not sure how much we're hiding, since the retry process is logged. I think > it's a question of whether a single setmeta call that fails should throw an > error, blocking all the others (in the case of wildcards, etc., unless this is > prevented elsewhere already) or should fail with a warning, which could be more > easily missed. > > I'm not sure what the right answer is based on gsutil's current architecture, > but I could see it going either way. If there's something in place to prevent > one failed update from breaking others that were part of the same call, my vote > would definitely be for making this an error. If it fails after N retries you definitely want to raise. gsutil's top-level contract is it exits with non-0 exit status in case an operation fails. (Good catch Travis.)
Sign in to reply to this message.
FWIW, the chacl command will eventually exit with a nonzero status if something failed but first will continue to try the rest of the URIs (whether they were many passed in, or from a wildcard). I'm not sure if I should change that, but I believe I modeled it after the way setacl works. Benson On Tue, Feb 12, 2013 at 2:37 PM, <mfschwartz@google.com> wrote: > > https://codereview.appspot.**com/7311079/diff/6002/src/** > gslib/commands/setmeta.py<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py> > File src/gslib/commands/setmeta.py (right): > > https://codereview.appspot.**com/7311079/diff/6002/src/** > gslib/commands/setmeta.py#**newcode242<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py#newcode242> > src/gslib/commands/setmeta.py:**242: return > On 2013/02/12 22:32:14, zwilt wrote: > >> On 2013/02/12 22:16:17, thobrla wrote: >> > Are we hiding error information here by returning instead of raising >> > once we > >> > give up? >> > > I'm not sure how much we're hiding, since the retry process is logged. >> > I think > >> it's a question of whether a single setmeta call that fails should >> > throw an > >> error, blocking all the others (in the case of wildcards, etc., unless >> > this is > >> prevented elsewhere already) or should fail with a warning, which >> > could be more > >> easily missed. >> > > I'm not sure what the right answer is based on gsutil's current >> > architecture, > >> but I could see it going either way. If there's something in place to >> > prevent > >> one failed update from breaking others that were part of the same >> > call, my vote > >> would definitely be for making this an error. >> > > If it fails after N retries you definitely want to raise. gsutil's > top-level contract is it exits with non-0 exit status in case an > operation fails. > (Good catch Travis.) > > https://codereview.appspot.**com/7311079/<https://codereview.appspot.com/7311... >
Sign in to reply to this message.
It's standard for unix commands to exit when they hit the first failure, and only continue over failures if specified in an explicit commandline option. We had a user ask about this on the gs-discussion list recently for one of the gsutil commands. I think chacl should exit immediately if there's a failure after all 3 retries (or if there are other failures, like permission denied). We could later add an option to continue in the face of failures. On Tue, Feb 12, 2013 at 3:39 PM, Benson Kalahar <bensonk@google.com> wrote: > FWIW, the chacl command will eventually exit with a nonzero status if > something failed but first will continue to try the rest of the URIs > (whether they were many passed in, or from a wildcard). I'm not sure if I > should change that, but I believe I modeled it after the way setacl works. > > Benson > > > On Tue, Feb 12, 2013 at 2:37 PM, <mfschwartz@google.com> wrote: > >> >> https://codereview.appspot.**com/7311079/diff/6002/src/** >> gslib/commands/setmeta.py<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py> >> File src/gslib/commands/setmeta.py (right): >> >> https://codereview.appspot.**com/7311079/diff/6002/src/** >> gslib/commands/setmeta.py#**newcode242<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py#newcode242> >> src/gslib/commands/setmeta.py:**242: return >> On 2013/02/12 22:32:14, zwilt wrote: >> >>> On 2013/02/12 22:16:17, thobrla wrote: >>> > Are we hiding error information here by returning instead of raising >>> >> once we >> >>> > give up? >>> >> >> I'm not sure how much we're hiding, since the retry process is logged. >>> >> I think >> >>> it's a question of whether a single setmeta call that fails should >>> >> throw an >> >>> error, blocking all the others (in the case of wildcards, etc., unless >>> >> this is >> >>> prevented elsewhere already) or should fail with a warning, which >>> >> could be more >> >>> easily missed. >>> >> >> I'm not sure what the right answer is based on gsutil's current >>> >> architecture, >> >>> but I could see it going either way. If there's something in place to >>> >> prevent >> >>> one failed update from breaking others that were part of the same >>> >> call, my vote >> >>> would definitely be for making this an error. >>> >> >> If it fails after N retries you definitely want to raise. gsutil's >> top-level contract is it exits with non-0 exit status in case an >> operation fails. >> (Good catch Travis.) >> >> https://codereview.appspot.**com/7311079/<https://codereview.appspot.com/7311... >> > >
Sign in to reply to this message.
Fixed the raise/return issue. Is there anything else blocking this, or should I go ahead and submit? https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py File src/gslib/commands/setmeta.py (right): https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.p... src/gslib/commands/setmeta.py:242: return On 2013/02/12 22:37:10, Mike Schwartz wrote: > On 2013/02/12 22:32:14, zwilt wrote: > > On 2013/02/12 22:16:17, thobrla wrote: > > > Are we hiding error information here by returning instead of raising once we > > > give up? > > > > I'm not sure how much we're hiding, since the retry process is logged. I think > > it's a question of whether a single setmeta call that fails should throw an > > error, blocking all the others (in the case of wildcards, etc., unless this is > > prevented elsewhere already) or should fail with a warning, which could be > more > > easily missed. > > > > I'm not sure what the right answer is based on gsutil's current architecture, > > but I could see it going either way. If there's something in place to prevent > > one failed update from breaking others that were part of the same call, my > vote > > would definitely be for making this an error. > > If it fails after N retries you definitely want to raise. gsutil's top-level > contract is it exits with non-0 exit status in case an operation fails. > (Good catch Travis.) Just fixed this to raise the error (which I like much better).
Sign in to reply to this message.
Just out of curiosity, how does this work with -m? Does the first thread to break kill all others, or do the others continue? On Tue, Feb 12, 2013 at 2:57 PM, Michael Schwartz <mfschwartz@google.com>wrote: > It's standard for unix commands to exit when they hit the first failure, > and only continue over failures if specified in an explicit commandline > option. We had a user ask about this on the gs-discussion list recently for > one of the gsutil commands. I think chacl should exit immediately if > there's a failure after all 3 retries (or if there are other failures, like > permission denied). We could later add an option to continue in the face of > failures. > > > On Tue, Feb 12, 2013 at 3:39 PM, Benson Kalahar <bensonk@google.com>wrote: > >> FWIW, the chacl command will eventually exit with a nonzero status if >> something failed but first will continue to try the rest of the URIs >> (whether they were many passed in, or from a wildcard). I'm not sure if I >> should change that, but I believe I modeled it after the way setacl works. >> >> Benson >> >> >> On Tue, Feb 12, 2013 at 2:37 PM, <mfschwartz@google.com> wrote: >> >>> >>> https://codereview.appspot.**com/7311079/diff/6002/src/** >>> gslib/commands/setmeta.py<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py> >>> File src/gslib/commands/setmeta.py (right): >>> >>> https://codereview.appspot.**com/7311079/diff/6002/src/** >>> gslib/commands/setmeta.py#**newcode242<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py#newcode242> >>> src/gslib/commands/setmeta.py:**242: return >>> On 2013/02/12 22:32:14, zwilt wrote: >>> >>>> On 2013/02/12 22:16:17, thobrla wrote: >>>> > Are we hiding error information here by returning instead of raising >>>> >>> once we >>> >>>> > give up? >>>> >>> >>> I'm not sure how much we're hiding, since the retry process is logged. >>>> >>> I think >>> >>>> it's a question of whether a single setmeta call that fails should >>>> >>> throw an >>> >>>> error, blocking all the others (in the case of wildcards, etc., unless >>>> >>> this is >>> >>>> prevented elsewhere already) or should fail with a warning, which >>>> >>> could be more >>> >>>> easily missed. >>>> >>> >>> I'm not sure what the right answer is based on gsutil's current >>>> >>> architecture, >>> >>>> but I could see it going either way. If there's something in place to >>>> >>> prevent >>> >>>> one failed update from breaking others that were part of the same >>>> >>> call, my vote >>> >>>> would definitely be for making this an error. >>>> >>> >>> If it fails after N retries you definitely want to raise. gsutil's >>> top-level contract is it exits with non-0 exit status in case an >>> operation fails. >>> (Good catch Travis.) >>> >>> https://codereview.appspot.**com/7311079/<https://codereview.appspot.com/7311... >>> >> >> >
Sign in to reply to this message.
Interrupting gsutil -m currently doesn't work well. There's an open issue<https://code.google.com/p/gsutil/issues/detail?id=100>about this. On Tue, Feb 12, 2013 at 4:21 PM, Zach Wilt <zwilt@google.com> wrote: > Just out of curiosity, how does this work with -m? Does the first thread > to break kill all others, or do the others continue? > > > On Tue, Feb 12, 2013 at 2:57 PM, Michael Schwartz <mfschwartz@google.com>wrote: > >> It's standard for unix commands to exit when they hit the first failure, >> and only continue over failures if specified in an explicit commandline >> option. We had a user ask about this on the gs-discussion list recently for >> one of the gsutil commands. I think chacl should exit immediately if >> there's a failure after all 3 retries (or if there are other failures, like >> permission denied). We could later add an option to continue in the face of >> failures. >> >> >> On Tue, Feb 12, 2013 at 3:39 PM, Benson Kalahar <bensonk@google.com>wrote: >> >>> FWIW, the chacl command will eventually exit with a nonzero status if >>> something failed but first will continue to try the rest of the URIs >>> (whether they were many passed in, or from a wildcard). I'm not sure if I >>> should change that, but I believe I modeled it after the way setacl works. >>> >>> Benson >>> >>> >>> On Tue, Feb 12, 2013 at 2:37 PM, <mfschwartz@google.com> wrote: >>> >>>> >>>> https://codereview.appspot.**com/7311079/diff/6002/src/** >>>> gslib/commands/setmeta.py<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py> >>>> File src/gslib/commands/setmeta.py (right): >>>> >>>> https://codereview.appspot.**com/7311079/diff/6002/src/** >>>> gslib/commands/setmeta.py#**newcode242<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py#newcode242> >>>> src/gslib/commands/setmeta.py:**242: return >>>> On 2013/02/12 22:32:14, zwilt wrote: >>>> >>>>> On 2013/02/12 22:16:17, thobrla wrote: >>>>> > Are we hiding error information here by returning instead of raising >>>>> >>>> once we >>>> >>>>> > give up? >>>>> >>>> >>>> I'm not sure how much we're hiding, since the retry process is logged. >>>>> >>>> I think >>>> >>>>> it's a question of whether a single setmeta call that fails should >>>>> >>>> throw an >>>> >>>>> error, blocking all the others (in the case of wildcards, etc., unless >>>>> >>>> this is >>>> >>>>> prevented elsewhere already) or should fail with a warning, which >>>>> >>>> could be more >>>> >>>>> easily missed. >>>>> >>>> >>>> I'm not sure what the right answer is based on gsutil's current >>>>> >>>> architecture, >>>> >>>>> but I could see it going either way. If there's something in place to >>>>> >>>> prevent >>>> >>>>> one failed update from breaking others that were part of the same >>>>> >>>> call, my vote >>>> >>>>> would definitely be for making this an error. >>>>> >>>> >>>> If it fails after N retries you definitely want to raise. gsutil's >>>> top-level contract is it exits with non-0 exit status in case an >>>> operation fails. >>>> (Good catch Travis.) >>>> >>>> https://codereview.appspot.**com/7311079/<https://codereview.appspot.com/7311... >>>> >>> >>> >> >
Sign in to reply to this message.
LGTM for submitting. Thanks. On Tue, Feb 12, 2013 at 4:19 PM, <zwilt@google.com> wrote: > Fixed the raise/return issue. Is there anything else blocking this, or > should I go ahead and submit? > > > > https://codereview.appspot.**com/7311079/diff/6002/src/** > gslib/commands/setmeta.py<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py> > File src/gslib/commands/setmeta.py (right): > > https://codereview.appspot.**com/7311079/diff/6002/src/** > gslib/commands/setmeta.py#**newcode242<https://codereview.appspot.com/7311079/diff/6002/src/gslib/commands/setmeta.py#newcode242> > src/gslib/commands/setmeta.py:**242: return > On 2013/02/12 22:37:10, Mike Schwartz wrote: > >> On 2013/02/12 22:32:14, zwilt wrote: >> > On 2013/02/12 22:16:17, thobrla wrote: >> > > Are we hiding error information here by returning instead of >> > raising once we > >> > > give up? >> > >> > I'm not sure how much we're hiding, since the retry process is >> > logged. I think > >> > it's a question of whether a single setmeta call that fails should >> > throw an > >> > error, blocking all the others (in the case of wildcards, etc., >> > unless this is > >> > prevented elsewhere already) or should fail with a warning, which >> > could be > >> more >> > easily missed. >> > >> > I'm not sure what the right answer is based on gsutil's current >> > architecture, > >> > but I could see it going either way. If there's something in place >> > to prevent > >> > one failed update from breaking others that were part of the same >> > call, my > >> vote >> > would definitely be for making this an error. >> > > If it fails after N retries you definitely want to raise. gsutil's >> > top-level > >> contract is it exits with non-0 exit status in case an operation >> > fails. > >> (Good catch Travis.) >> > > Just fixed this to raise the error (which I like much better). > > https://codereview.appspot.**com/7311079/<https://codereview.appspot.com/7311... >
Sign in to reply to this message.
|
