Do more retries on unexpected tree service responses #174

alexvanin · 2023-10-05T12:52:30Z

alexvanin commented

2023-10-05 12:52:30 +00:00

Closes #172

Try its best by looking for nodes during 'GetNodeByPath'
Retry on 'tree not found' and other not found errors

Closes #172 1. Try its best by looking for nodes during 'GetNodeByPath' 2. Retry on 'tree not found' and other not found errors

alexvanin self-assigned this 2023-10-05 12:52:31 +00:00

requested reviews from fyrchik, dkirillov, mbiryukova

2023-10-05 12:52:52 +00:00

mbiryukova approved these changes 2023-10-05 13:24:15 +00:00

fyrchik approved these changes 2023-10-05 14:40:54 +00:00

dkirillov reviewed 2023-10-05 14:51:20 +00:00

dkirillov reviewed 2023-10-05 14:58:21 +00:00

pool/tree/pool.go Outdated

					
				@ -299,2 +305,3 @@

						}

						return handleError("failed to get node by path", inErr)

					}); err != nil {

					}); err != nil && !errors.Is(err, ErrNodeNotFound) {

dkirillov commented

2023-10-05 14:58:21 +00:00

It seems if all nodes return for example tree not found error because of this check we start return no error below (return resp.GetBody().GetNodes(), nil where resp is nil ) though we have actually got error from each client. Is it ok or am I missing something?

It seems if all nodes return for example `tree not found` error because of this check we start return no error below (`return resp.GetBody().GetNodes(), nil` where `resp` is `nil` ) though we have actually got error from each client. Is it ok or am I missing something?

alexvanin commented

2023-10-06 08:02:29 +00:00

Good point. At first I didn't reuse ErrNodeNotFound and created new error for that but then decided there is no reason to do that. So there is an actual reason to avoid ErrNodeNotFound reusage, thank you.

Good point. At first I didn't reuse `ErrNodeNotFound` and created new error for that but then decided there is no reason to do that. So there is an actual reason to avoid `ErrNodeNotFound` reusage, thank you.

dkirillov marked this conversation as resolved

alexvanin force-pushed feature/172 from 8cafee851c to eb5288f4a5

2023-10-06 08:13:03 +00:00

Compare

dkirillov reviewed 2023-10-06 08:24:00 +00:00

pool/tree/pool.go

					
				@ -752,3 +762,1 @@

					return !(err == nil ||

						errors.Is(err, ErrNodeNotFound) ||

						errors.Is(err, ErrNodeAccessDenied))

					return !(err == nil || errors.Is(err, ErrNodeAccessDenied))

dkirillov commented

2023-10-06 08:24:00 +00:00

It seems now we have to add errNodeEmptyResult here

It seems now we have to add `errNodeEmptyResult` here

alexvanin commented

2023-10-06 09:34:54 +00:00

Disagree, because shouldTryAgain describes errors which avoid retry:

when error is nil, so no error, no retry
when access is denied which is an error unrelated to tree state, no retry.

errNodeEmptyResult should trigger retry, so we don't mention it there.

Disagree, because `shouldTryAgain` describes errors which avoid retry: - when error is nil, so no error, no retry - when access is denied which is an error unrelated to tree state, no retry. `errNodeEmptyResult` should trigger retry, so we don't mention it there.

dkirillov commented

2023-10-06 12:33:29 +00:00

Oh, I see

dkirillov marked this conversation as resolved

dkirillov approved these changes 2023-10-06 12:33:46 +00:00

alexvanin merged commit eb5288f4a5 into master

2023-10-06 12:53:27 +00:00

alexvanin deleted branch feature/172

2023-10-06 12:53:28 +00:00

Sign in to join this conversation.

No reviewers