migrate retryCallCluster for new ES client

[pgayvallet]

Summary

Part of #35508
Migrate retryCallCluster and migrationRetryCallCluster to be usable with new ES client.

Checklist

• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios

[pgayvallet]

This is a direct adaptation of

kibana/src/core/server/elasticsearch/legacy/retry_call_cluster.ts

Lines 62 to 74 in 159369b

	() => {
	return (
	error instanceof esErrors.NoConnections ||
	error instanceof esErrors.ConnectionFault ||
	error instanceof esErrors.ServiceUnavailable ||
	error instanceof esErrors.RequestTimeout ||
	error instanceof esErrors.AuthenticationException ||
	error instanceof esErrors.AuthorizationException ||
	// @ts-expect-error
	error instanceof esErrors.Gone ||
	error?.body?.error?.type === 'snapshot_in_progress_exception'
	);
	},

@delvedor Could you take a look see if I did not do any mapping mistake?

[delvedor]

The legacy client has a bug, where both the HTTP timeout error (408) and the socket timeout error will fall into the RequestTimeout error.
In the new client, the socket timeout is TimeoutError, while the HTTP timeout is a ResponseError with the statusCode property set to 408.
Not sure which one are you checking here.

[pgayvallet]

I'd say we probably want both. @rudolf can you confirm?

[rudolf]

Yes, we want to retry all operations, regardless of where the timeout occurred.

[delvedor]

From a performance point of view, instead of checking via instanceof, you could use error.name === {name}, which is an order of magnitude faster :)
Each error object of the client has a corresponding .name property: https://github.com/elastic/elasticsearch-js/blob/master/lib/errors.js

[mshustov]

Could we delegate type inference to the apiCaller?

export const retryCallCluster = <T extends Promise<unknown>>(apiCaller: () => T): T => {
  return defer(apiCaller)
    .pipe(
      retryWhen((errors) =>
        errors.pipe(
          concatMap((error, i) =>
            iif(
              () => error instanceof esErrors.NoLivingConnectionsError,
              timer(1000),
              throwError(error)
            )
          )
        )
      )
    )
    .toPromise() as T;
};

[pgayvallet]

Yea, I thought I had it with my APICaller type, but nope. Fixed.

[mshustov]

nit: i is not used

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[delvedor]

Note, if the status code of a response is 502, 503 or 504, the client performs an automatic failover internally, so have it here could be a duplication.
Regarding 4xx status codes, the client does not perform any automatic failover/retry.

[pgayvallet]

Good to know. Having this feature for our 'end' consumers is great. For the migration needs, we need to perform an infinity of retries, so keeping the logic in a single place is probably better. But we don't need to disable the internal failover here though, the duplication is probably fine.

[mshustov]

minor: it's not type safe anymore

[pgayvallet]

true, however the unit test are asserting behavior against concrete error from the library, so I guess this is alright. But I can revert the change if we prefer instanceof based checks.

[mshustov]

Nope, that optional. Feel free to merge

[mshustov]

Optional: I usually use chain of mockImplementationOnce

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ed94396

Build metrics

✅ unchanged

History

• 💚 Build #62710 succeeded 5e02511
• 💚 Build #61952 succeeded 85c77b9
• 💛 Build #60914 was flaky 1babcb4
• 💚 Build #60827 succeeded 5d8bbba
• 💛 Build #60816 was flaky 68c1bd3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream