Arvados

- Branch 7546-put-retry at commit 08a15a1e6b8e6f2e44f18328c1f6dd25343cffc2 is ready for review.

- I added the server to retry list if "status.statusCode 408 || status.statusCode 429 || status.statusCode >= 500" (using the list in GetOrHead). Do we need to include statusCode == 0 in this list? It appears that using a closed connection results in status.statusCode of 0 at line #205 of uploadToKeepServer method and this may need to be retried?

- One observation as I was working on putReplicas method: The requestId calculation at the beginning of this method uses "locator", which is not yet set. Shouldn't it be using "hash" instead of "locator" here?

 requestId := fmt.Sprintf("%x", md5.Sum([]byte(locator+time.Now().String())))[0:8] 

Radhika Chippada wrote:

- Branch 7546-put-retry at commit 08a15a1e6b8e6f2e44f18328c1f6dd25343cffc2 is ready for review.

- I added the server to retry list if "status.statusCode 408 || status.statusCode 429 || status.statusCode >= 500" (using the list in GetOrHead). Do we need to include statusCode 0 in this list? It appears that using a closed connection results in status.statusCode of 0 at line #205 of uploadToKeepServer method and this may need to be retried?

Yes, it should retry on statusCode 0 because that indicates a network error, which we definitely want to retry.

In addition, it probably should not retry on status code 503, because that's the error returned by keepstore when the keep server is full and cannot accept any more blocks.

- One observation as I was working on putReplicas method: The requestId calculation at the beginning of this method uses "locator", which is not yet set. Shouldn't it be using "hash" instead of "locator" here?

[...]

Yes, that looks like a bug.

Peter,
Addressed those comments.

One other observation / thought. The tests are now taking longer to run, which is understandable because now we are retrying. However, I am wondering if it is agreeable to set retry count to 0 for tests where retry is not needed / not going to make any difference one way or the other. Example of one such test seems to be: TestPutWithTooManyFail

Thanks.

Updated three tests to use kc.Retries = 0 (where retries are not needed). Now the test run time has come down from current master 49s to 10s.

Could you tweak the comment "// Timeout, too many requests, or other server side failure" to mention that error 503 means "keep server full" so we don't want to retry that case. Then please go ahead and merge.