fix problem with mv -m not percolating exceptions from cp phase

[Mike Schwartz, 2012-02-20 16:12:17]

Since this is an important fix, can you please implement an mv test that forces
a fail during the copy, and asserts that the original file didn't get deleted,
with the -m option in force?

http://codereview.appspot.com/5688051/diff/1/gslib/command.py
File gslib/command.py (right):

http://codereview.appspot.com/5688051/diff/1/gslib/command.py#newcode638
gslib/command.py:638: # process, increment value for each shared variable.
Missing ')'

http://codereview.appspot.com/5688051/diff/1/gslib/commands/cp.py
File gslib/commands/cp.py (right):

http://codereview.appspot.com/5688051/diff/1/gslib/commands/cp.py#newcode816
gslib/commands/cp.py:816: raise CommandException('Some files could not be
transferred.')
This message is slightly too specific, since for copy-in-the-cloud there are
only objects, not files. How about changing it to "files/objects".

Also, since you now have a count, why not print the count here (in which case I
think line 800 can be dropped)?

[marccohen, 2012-02-20 19:24:06]

In addition to fixing your comments, I realized that my fix only catches
exceptions raised in the function that gets applied by each thread but if
there's an exception raised in the sub-process logic outside of the applied
function (e.g. a bug in the _ApplyThread method itself) then I won't catch it so
I added some additional logic here to abort the main proc if one or more
sub-procs returns a non-zero exit code (i.e if there's an exception not caught
by the applied function).

Re: your suggestion to write an automated test, in my unit tests I specified
multiple files (to force sub-procs) and -m, made one of the src URIs
non-existent and verified there was no attempt to remove anything (i.e. the copy
stage failed globally). Would that be sufficient for an automated test? I
realize a better test would be to force a failure and verify the source file
doesn't get removed but I'm struggling with how to do that without inserting
special code to force a failure artificially.  Any thoughts?

http://codereview.appspot.com/5688051/diff/1/gslib/command.py
File gslib/command.py (right):

http://codereview.appspot.com/5688051/diff/1/gslib/command.py#newcode638
gslib/command.py:638: # process, increment value for each shared variable.
On 2012/02/20 16:12:17, Mike Schwartz wrote:
> Missing ')'

Done.

http://codereview.appspot.com/5688051/diff/1/gslib/commands/cp.py
File gslib/commands/cp.py (right):

http://codereview.appspot.com/5688051/diff/1/gslib/commands/cp.py#newcode816
gslib/commands/cp.py:816: raise CommandException('Some files could not be
transferred.')
On 2012/02/20 16:12:17, Mike Schwartz wrote:
> This message is slightly too specific, since for copy-in-the-cloud there are
> only objects, not files. How about changing it to "files/objects".
> 
> Also, since you now have a count, why not print the count here (in which case
I
> think line 800 can be dropped)?

Done.

[Mike Schwartz, 2012-02-20 20:09:26]

On 2012/02/20 19:24:06, marccohen wrote:
> In addition to fixing your comments, I realized that my fix only catches
> exceptions raised in the function that gets applied by each thread but if
> there's an exception raised in the sub-process logic outside of the applied
> function (e.g. a bug in the _ApplyThread method itself) then I won't catch it
so
> I added some additional logic here to abort the main proc if one or more
> sub-procs returns a non-zero exit code (i.e if there's an exception not caught
> by the applied function).

Did you add this test to the review? I don't see any test code in here.

> Re: your suggestion to write an automated test, in my unit tests I specified
> multiple files (to force sub-procs) and -m, made one of the src URIs
> non-existent and verified there was no attempt to remove anything (i.e. the
copy
> stage failed globally). Would that be sufficient for an automated test? I
> realize a better test would be to force a failure and verify the source file
> doesn't get removed but I'm struggling with how to do that without inserting
> special code to force a failure artificially.  Any thoughts?

Oh, right, I was thinking about the test harnass I implemented in the resumable
upload tests, that allows forcing failures during uploads, but that's in a
separate piece of code. What you have is probably good enough.

> http://codereview.appspot.com/5688051/diff/1/gslib/command.py
> File gslib/command.py (right):
> 
> http://codereview.appspot.com/5688051/diff/1/gslib/command.py#newcode638
> gslib/command.py:638: # process, increment value for each shared variable.
> On 2012/02/20 16:12:17, Mike Schwartz wrote:
> > Missing ')'
> 
> Done.
> 
> http://codereview.appspot.com/5688051/diff/1/gslib/commands/cp.py
> File gslib/commands/cp.py (right):
> 
> http://codereview.appspot.com/5688051/diff/1/gslib/commands/cp.py#newcode816
> gslib/commands/cp.py:816: raise CommandException('Some files could not be
> transferred.')
> On 2012/02/20 16:12:17, Mike Schwartz wrote:
> > This message is slightly too specific, since for copy-in-the-cloud there are
> > only objects, not files. How about changing it to "files/objects".
> > 
> > Also, since you now have a count, why not print the count here (in which
case
> I
> > think line 800 can be dropped)?
> 
> Done.

[marccohen, 2012-02-20 20:33:33]

Thanks. One more thing re: this issue:

> Re: your suggestion to write an automated test, in my unit tests I specified
> multiple files (to force sub-procs) and -m, made one of the src URIs
> non-existent and verified there was no attempt to remove anything (i.e. the
copy
> stage failed globally). Would that be sufficient for an automated test? I
> realize a better test would be to force a failure and verify the source file
> doesn't get removed but I'm struggling with how to do that without inserting
> special code to force a failure artificially.  Any thoughts?


Oh, right, I was thinking about the test harnass I implemented in the resumable
upload tests, that allows forcing failures during uploads, but that's in a
separate piece of code. What you have is probably good enough.


The unit test I referred to was a manual test and I agree with you that an
automated regression test is called for here. If it's ok with you I'll add
an automated test that does what my manual unit test did - mv multiple
source objects, where one doesn't exist, and verify the remove stage is not
done for any objects, by grepping the command output.

Marc

[marccohen, 2012-02-20 21:45:54]

Using my new test capability, added automatic testing of parallel (-m) mv
command, verifying that there's no attempt to remove anything when errors occur
in source URIs. Transcript follows...

marccohen@mcohen:~/gsutil/gsutil-excfix/src$ ./gsutil test mv
Global setup started...
Global setup completed.
Running tests for mv command.
gen expect files ... ok
verify 2 src objs ... ok
verify 0 dst objs ... ok
mv 2 objects ... Removing
gs://gsutil_test_bucket_marccohen_2/gsutil_test_object_marccohen_0...
Removing gs://gsutil_test_bucket_marccohen_2/gsutil_test_object_marccohen_1...
ok
verify 0 src objs ... ok
verify 2 dst objs ... ok
rm 1 src object ... ok
verify 1 src obj ... ok
verify 0 dst objs ... ok
mv 2 objects ... ok

----------------------------------------------------------------------
Ran 10 tests in 3.712s

OK
Global teardown started...
Global teardown completed.

[Mike Schwartz, 2012-02-20 21:59:24]

Nice. Thanks Marc.

Mike


On Mon, Feb 20, 2012 at 1:45 PM, <marccohen@google.com> wrote:

> Using my new test capability, added automatic testing of parallel (-m)
> mv command, verifying that there's no attempt to remove anything when
> errors occur in source URIs. Transcript follows...
>
> marccohen@mcohen:~/gsutil/**gsutil-excfix/src$ ./gsutil test mv
> Global setup started...
> Global setup completed.
> Running tests for mv command.
> gen expect files ... ok
> verify 2 src objs ... ok
> verify 0 dst objs ... ok
> mv 2 objects ... Removing
> gs://gsutil_test_bucket_**marccohen_2/gsutil_test_**object_marccohen_0...
> Removing
> gs://gsutil_test_bucket_**marccohen_2/gsutil_test_**object_marccohen_1...
> ok
> verify 0 src objs ... ok
> verify 2 dst objs ... ok
> rm 1 src object ... ok
> verify 1 src obj ... ok
> verify 0 dst objs ... ok
> mv 2 objects ... ok
>
> ------------------------------**------------------------------**----------
> Ran 10 tests in 3.712s
>
> OK
> Global teardown started...
> Global teardown completed.
>
>
>
http://codereview.appspot.com/**5688051/<http://codereview.appspot.com/5688051/>
>

[Mike Schwartz, 2012-02-22 17:50:10]

Sending a test comment to test Rietveld notification

[marccohen, 2012-02-22 17:50:55]

I got it.

On Wed, Feb 22, 2012 at 9:50 AM, <mfschwartz@google.com> wrote:

> Sending a test comment to test Rietveld notification
>
>
http://codereview.appspot.com/**5688051/<http://codereview.appspot.com/5688051/>
>