In particular dmADB.pushFile() and dmADB.pushDir() will sometimes hit an adb error where adb will hang and the command will timeout. We have a whole bunch of places throughout the B2G automation code to get around this by retrying the push if it times out. e.g: http://hg.mozilla.org/mozilla-central/file/6c23f41b0747/testing/marionette/client/marionette/emulator.py#l392 I would be happy if this got added to just pushFile and pushDir, but it seems like a retry option might be good to add to several of the other commands in both implementations as well.

Note that dmSUT already does retry automatically several times in case of error, with the exception of pullFile. This is even configurable by modifying the internal "retrylimit" variable. See for example: https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanagerSUT.py#L113 For dmADB, I agree this would be a good idea.

Like this? I tried to follow dmSUT's pattern.

Comment on attachment 689704 [details] [diff] [review] implement retry functionality in _checkCmd Review of attachment 689704 [details] [diff] [review]: ----------------------------------------------------------------- I guess it depends how much work you want to do :p. On one hand I'd say this is fine because it's how dmSUT works, on the other I can't say I'm a fan of how dmSUT works. We found the larger the file you are pushing, the more likely it is to fail. For that reason I'd rather see a retries parameter that you can pass in per function that defaults to 1 (or maybe that defaults to self.retrylimit). Just personal preference, but doing it this way is quicker and easier so I'd be fine with it for now. I'll f+ and leave it up to you

(In reply to Andrew Halberstadt [:ahal] from comment #3) > Comment on attachment 689704 [details] [diff] [review] > implement retry functionality in _checkCmd > > Review of attachment 689704 [details] [diff] [review]: > ----------------------------------------------------------------- > > I guess it depends how much work you want to do :p. On one hand I'd say this > is fine because it's how dmSUT works, on the other I can't say I'm a fan of > how dmSUT works. We found the larger the file you are pushing, the more > likely it is to fail. For that reason I'd rather see a retries parameter > that you can pass in per function that defaults to 1 (or maybe that defaults > to self.retrylimit). Just personal preference, but doing it this way is > quicker and easier so I'd be fine with it for now. I'd prefer if we didn't introduce any more confusing differences between dmADB and dmSUT. The patch just posted is fine, but adding a retries parameter to each function is something I'd want in both implementations.

:ahal, like this? :wlach, I guess we could add the retrylimit kwarg in the abstract methods in dm.py?

(In reply to William Lachance (:wlach) from comment #4) > I'd prefer if we didn't introduce any more confusing differences between > dmADB and dmSUT. The patch just posted is fine, but adding a retries > parameter to each function is something I'd want in both implementations. Agreed. Which is why I would be fine with just landing this as is for now (the other way would involve more changes). Sorry, should have made that more clear. Mihnea, however if you are up to it, feel free to make the changes across the board

(In reply to Andrew Halberstadt [:ahal] from comment #6) > Mihnea, however if you are up to it, feel free to make the changes across > the board Sure, so basically this means having the retrylimit kwarg in the method prototypes for pushFile, pushDir and the corresponding implementations of "checkCmd" (or however they are called in dmSUT). Something similar to my latest patch but replicated for dmSUT and reflected in the abstract dm, right? Also, do you mind changing retrylimit to retry_limit? :)

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #7) > Sure, so basically this means having the retrylimit kwarg in the method > prototypes for pushFile, pushDir and the corresponding implementations of > "checkCmd" (or however they are called in dmSUT). Something similar to my > latest patch but replicated for dmSUT and reflected in the abstract dm, > right? Yep, sounds about right > Also, do you mind changing retrylimit to retry_limit? :) Please!

(In reply to Andrew Halberstadt [:ahal] from comment #8) > > Also, do you mind changing retrylimit to retry_limit? :) > > Please! For things like this we should be using the style in the surrounding module, which would mean a variable named retryLimit (retrylimit is probably close enough).

(In reply to William Lachance (:wlach) from comment #9) > (In reply to Andrew Halberstadt [:ahal] from comment #8) > > > > Also, do you mind changing retrylimit to retry_limit? :) > > > > Please! > > For things like this we should be using the style in the surrounding module, > which would mean a variable named retryLimit (retrylimit is probably close > enough). You are right, I forgot about the Java-ness of dm.

Comment on attachment 689801 [details] [diff] [review] add retryLimit to dm, dmADB and dmSUT in pushFile and pushDir Review of attachment 689801 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I don't think 'self' will be defined in the method signature though. I can change to r+ with that fixed. ::: mozdevice/mozdevice/devicemanagerADB.py @@ +164,4 @@ > def _disconnectRemoteADB(self): > self._checkCmd(["disconnect", self.host + ":" + str(self.port)]) > > + def pushFile(self, localname, destname, retryLimit=self.retryLimit): Pretty sure this won't work. You'll need to do something like 'retryLimit = retryLimit or self.retryLimit' @@ +735,5 @@ > + proc.kill() > + retries += 1 > + continue > + return ret_code > + raise DMError("Timeout exceeded for _checkCmd call for %d retries." % retries) nit: s/call for/call after

Fixed

Comment on attachment 689809 [details] [diff] [review] add retryLimit to dm, dmADB and dmSUT in pushFile and pushDir Review of attachment 689809 [details] [diff] [review]: ----------------------------------------------------------------- Cool, thanks!

I filed bug 819482 to remove the code we had in the B2G automation to do this

I'm not familiar with the current policy on where the dm code should be merged first (mozbase@github vs m-c). Will you take care of the merge?

Sure. I want to run it through try first to be safe. But normally we just land in github and it will get synced to m-c whenever.

try: https://tbpl.mozilla.org/?tree=Try&rev=e6fa566d858b

Looks like we missed a parameter: https://tbpl.mozilla.org/php/getParsedLog.php?id=17722618&tree=Try

oops, fixed that.

Pushed to try again: https://hg.mozilla.org/try/rev/48bf2fa5b665

https://github.com/mozilla/mozbase/commit/93701293b5fa5ea538faec9d7f3ba6abcdf4ac48 I think we can wait for the next mozbase -> m-c, no need to cherry pick this.