LLRC: Make warning behavior pluggable per request #36345
Conversation
Adds the ability for individual requests to override strict deprecation warnings. It should also be useful for folks that want to run with strict deprecation enabled but need to make a few deprecated calls in their application. For those folks it could be part of a migration to strict deprecation. It should also allow us to write much more precise tests than we do now. In particular, it'll let us run all of our tests with strict deprecation enabled and disable it on just the requests that we expect to be deprecated. To start us down that path, this depreates `ESRestTestCase#getStrictDeprecationMode`, pointing contributors to the override. It also removes the yaml test's override of `getStrictDeprecationMode` and replaces it with the migration to prove that we can do it. I thought a fair bit about whether or not this override should be on `Request` or `RequestOptions` but I landed on `RequestOptions` because it seems like something that you'd want to do on a whole class of requests across your application. Which is sort of what `RequestOptions` is for, being immutable with a builder and stuff. Relates to elastic#36044
This allows you to plug the behavior that the LLRC uses to handle warnings on a per request basis. We entertained the idea of allowing you to set the warnings behavior to strict mode on a per request basis but that wouldn't allow the high level rest client to fail when it sees an unexpected warning. We also entertained the idea of adding a list of "required warnings" to the `RequestOptions` but that won't work well with failures that occur *sometimes* like those we see in mixed clusters. Adding a list of "allowed warnings" to the `RequestOptions` would work for mixed clusters but it'd leave many of the assertions in our tests weaker than we'd like. This behavior plugging implementation allows us to make a "required warnings" option when we need it and an "allowed warnings" behavior when we need it. I don't think this behavior is going to be commonly used by used outside of the Elasticsearch build, but I expect they'll be a few commendably paranoid folks who could use this behavior.
nik9000
added
>feature
:Clients/Java Low Level REST Client
|
Pinging @elastic/es-core-features |
| /** | ||
| * Handler for warnings or null if the request should use | ||
| * {@link RestClientBuilder#setStrictDeprecationMode}. | ||
| */ |
There was a problem hiding this comment.
I find myself having to think a lot about this sentence. Almost like it needs brackets to parse the logic. Maybe I just need another coffee.
There was a problem hiding this comment.
I'll push some better words.
| && thisWarningsHandler.warningsShouldFailRequest(response.getWarnings())) { | ||
| listener.onDefinitiveFailure(new ResponseException(response)); | ||
| } else { | ||
| listener.onSuccess(response); |
There was a problem hiding this comment.
Here we lose the option to fail if an expected warning didn't materialise. Is that OK? Its conceivable some tests can be certain in their expectations of failure and we do need a way to fail when we fail to fail (so to speak).
| * behavior on individual requests | ||
| */ | ||
| @Deprecated | ||
| protected boolean getStrictDeprecationMode() { |
There was a problem hiding this comment.
Got it. Now I see the relationship between WarningsHandler and setStrictDeprecationMode. Maybe make that earlier Javadoc more explicit about the fact setStrictDeprecationMode has been deprecated?
There was a problem hiding this comment.
I don't think RestClientBuilder#setStrictDeprecationMode has been deprecated in this PR but maybe it should be. Or we could do it as a followup.
There was a problem hiding this comment.
I don't feel to bad about having strict deprecation mode on the builder and this more expressive WarningsHandler thing on the RequestOptions. I don't think setting a WarningsHandler on the builder makes sense because it is about being careful and you can't really be careful for all of the requests that a client makes.
|
Looks good, Nik. I made some comments - perhaps the most important is whether failing due to the absence of warnings should be a possibility. HLRC clients don't have the luxury of seeing an LLRC response to make these assertions so handling via WarningsHandler would potentially be useful. For the types removal work I'd still like to wrap up a helper function similar to the "expectWarnings" in my PR (simply list an array of expected warning strings) but I can add that in a PR that builds on the more capable framework provided here. |
There was a problem hiding this comment.
I am good with this. I wish that we could find a way to check warnings without effectively disabling strictness and relying on each test to do the right assertions. Yet I see how it gets complicated in mixed clusters environments. Though I also think that if we want to write good tests for warnings, maybe they should run against a single node? Or use a node selector to make sure that we send requests to the right nodes and we know what to expect? I think I need to see how this is going to be applied in practice but I think it's a great start and it also exposes the same functionality to the high-level REST client which is good too.
| * behavior on individual requests | ||
| */ | ||
| @Deprecated | ||
| protected boolean getStrictDeprecationMode() { |
There was a problem hiding this comment.
I don't think RestClientBuilder#setStrictDeprecationMode has been deprecated in this PR but maybe it should be. Or we could do it as a followup.
This is more for cases where we bump into warnings in the course of "normal" testing. We don't want to bump into the warnings but we have to. In this case I say we should be as strict as possible on which warnings we see so that if the warnings vanish we can remove the exception. There are some cases where we can match exactly (Mark's case) and there are some where we might see the warning and we might not (my case).
We can't use the node selector for rest test cleanup actions because the sniffer is incompatible with cloud and it'd make every test incompatible with cloud. We could fix the sniffer but that seems like a project that we don't have time for now. |
This allows you to plug the behavior that the LLRC uses to handle warnings on a per request basis. We entertained the idea of allowing you to set the warnings behavior to strict mode on a per request basis but that wouldn't allow the high level rest client to fail when it sees an unexpected warning. We also entertained the idea of adding a list of "required warnings" to the `RequestOptions` but that won't work well with failures that occur *sometimes* like those we see in mixed clusters. Adding a list of "allowed warnings" to the `RequestOptions` would work for mixed clusters but it'd leave many of the assertions in our tests weaker than we'd like. This behavior plugging implementation allows us to make a "required warnings" option when we need it and an "allowed warnings" behavior when we need it. I don't think this behavior is going to be commonly used by used outside of the Elasticsearch build, but I expect they'll be a few commendably paranoid folks who could use this behavior.
6 participants
This allows you to plug the behavior that the LLRC uses to handle
warnings on a per request basis.
We entertained the idea of allowing you to set the warnings behavior to
strict mode on a per request basis but that wouldn't allow the high
level rest client to fail when it sees an unexpected warning.
We also entertained the idea of adding a list of "required warnings" to
the
RequestOptionsbut that won't work well with failures that occursometimes like those we see in mixed clusters.
Adding a list of "allowed warnings" to the
RequestOptionswould workfor mixed clusters but it'd leave many of the assertions in our tests
weaker than we'd like.
This behavior plugging implementation allows us to make a "required
warnings" option when we need it and an "allowed warnings" behavior when
we need it.
I don't think this behavior is going to be commonly used by used outside
of the Elasticsearch build, but I expect they'll be a few commendably
paranoid folks who could use this behavior.