Extend ring.DoBatch's error class tracking to all errors

[duricanikolic]

What this PR does:

Error class tracking has been added to ring.DoBatch() in #201. That PR introduced 2 classes of errors:

• client errors (tracked by itemTracker.failed4xx), and
• server errors (tracked by itemTracker.failed5xx).

Unfortunately, this change took into account only errors with an HTTP status. In case of a gRPC error, or any other error not implementing GRPCStatus() *Status from google.golang.org/grpc/status, the error was implicitly treated as a server error. For example, if ring instances from a replication set return gRPC errors with gRPC codes FailedPrecondition (due to a client problem), FailedPrecondition (do to a client problem) and Unavailable (do to a server being currently unavailable), the current approach would return the last error that arrived, i.e., the server Unavailable error, although the majority of the instances would have given the client FailedPrecondition error.

Main change

This PR fixes that problem by introducing a new function,

ring.DoBatchWithClientError()

which is a generalization of ring.DoBatch() that has an additional parameter isClientError, a function specified by the caller that differentiates between client and server errors.

Minor changes

itemTracker's attributes failed4xx and failed5xx have been renamed to failedClient and failedServer.

Which issue(s) this PR fixes:

Part of grafana/mimir#6008

Checklist

• Tests updated
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[dimitarvdimitrov]

Thanks for working on this. I left a few comments, but the change looks mostly good

[dimitarvdimitrov]

what's the reason for this change?

[duricanikolic]

My idea was to set the new done attribute of an itemTracker to true as soon as that itemTracker reaches either its minSuccess or its maxFailure. Once it is achieved, we don't need to consider that itemTracker anymore.

[duricanikolic]

As discussed via slack, this will be done in a separate PR, and maybe in a different form. I have reverted this part of the PR.

[pstibrany]

I would suggest simpler design: passing a function to DoBatch that categorises errors to server and client-side, or perhaps does the merging of errors. Default function provided in dskit can work with http status codes, while function passed from Mimir can do its own error categorization. WDYT?

[dimitarvdimitrov]

the tests look better as table tests 👌

[dimitarvdimitrov]

I would suggest simpler design: passing a function to DoBatch that categorises errors to server and client-side, or perhaps does the merging of errors. Default function provided in dskit can work with http status codes, while function passed from Mimir can do its own error categorization. WDYT?

Passing a function will probably create fewer allocations for the client of the library.

But I feel like the interface of DoBatch will become more complex. It already takes one function argument which returns an error. Now it will have another func arg which will receive the same error and return a boolean. I feel like an optional interface on the returned error is less invasive and more straight forward.

[pstibrany]

But I feel like the interface of DoBatch will become more complex. It already takes one function argument which returns an error. Now it will have another func arg which will receive the same error and return a boolean. I feel like an optional interface on the returned error is less invasive and more straight forward.

DoBatch signature may not be nice, although we could simplify that. Using function would centralize all this logic into single place, with optional interface there will be mix of errors used all over the code base, some implementing the interface and some not.

[duricanikolic]

But I feel like the interface of DoBatch will become more complex. It already takes one function argument which returns an error. Now it will have another func arg which will receive the same error and return a boolean. I feel like an optional interface on the returned error is less invasive and more straight forward.

DoBatch signature may not be nice, although we could simplify that. Using function would centralize all this logic into single place, with optional interface there will be mix of errors used all over the code base, some implementing the interface and some not.

I have opted for this solution.

[dimitarvdimitrov]

thanks for addressing my comments

[pstibrany]

Thank you. Left some small suggestions for consideration.