Arvados

Idea: Should _intercept_http_request in api.py retry socket.error the same way it retries BadStatusLine? The comments for that logic suggest the rationale is nearly identical.

https://github.com/jcgregorio/httplib2/issues/240 suggests there is no way to close the socket explicitly.

I have a branch up on qr1hi that implements the planned fix, and I'm running the known-failing job with it.

Update: I have had to update basically everything around the failing job—run-command, the Docker image—to get a valid test going. Still chipping at it.

qr1hi-8i9sb-ar91eq68o7afgzx succeeded with the patch.

For background, it might also help to look at httplib2's conn_request method_init__.py#L1265. You can compare how it handles and retries different exceptions, including socket.error, and what we do in _intercept_http_request.

Note that httplib2 itself only retries socket errors that are ENETUNREACH or EADDRNOTAVAIL. Turning on its retry support wouldn't help us address the cases we've seen, which are EPIPE and ETIMEOUT.

reviewing 6a5cfd533c78824f65a9cf91e722570278d955e7

1. for better understanding of the nature of the exception will be good if the exception.message gets logged. _logger.warning(e.message) before returning.

2. as a (maybe later) improvement debugging before conn.close() will be good to have the list of connections with _logger.debug()

eyeballing the code I cant come up with any problems that might rise right away. Tests pass (even though this isn't tested)

1. and 2. aren't required for the merge in my opinion, but they are nice features to have (specially for OPS). LGTM

Nico Cesar wrote:

reviewing 6a5cfd533c78824f65a9cf91e722570278d955e7

1. for better understanding of the nature of the exception will be good if the exception.message gets logged. _logger.warning(e.message) before returning.

I added this at the debug level, so you can see it if you run with ARVADOS_DEBUG=1. If this retry does end up being routine—which it's starting to look like it might be—I didn't think it would be helpful to clutter job logs with it by logging a warning.

2. as a (maybe later) improvement debugging before conn.close() will be good to have the list of connections with _logger.debug()

I think if we're worried about this we should investigate it interactively with pdb or something like that. To my mind, the only question is, am I right that the API server connection is the only one? If I am, that's all we need. If not, we need to consider different code entirely. I'll note some of the other things we're doing in that function, like adding the Authorization header, are also inappropriate if the client makes connections to other servers. So the code already makes the same assumption and needs more fixes if it's wrong.

Merged this. I'll work on testing it next. Thanks.

7587-pysdk-retry-test-wip is up for review. It has two commits. The first fixes a bug where the API client tests could talk to the API server configured in the environment, even a real one. See the commit log for more background and rationale. That commit only fixes that issue.

The second commit adds the actual test.

7587-pysdk-retry-test-wip @ cc5c45d LGTM