REST client: introduce a strict deprecation mode #33708

lipsill · 2018-09-14T10:02:31Z

Minimal change that marks a rest request as failed if a deprecation
warning has been issued by the server. This is valuable for internal
tests and documentation snippets so that only non-deprecated code is
used.
This also minimizes possible divergence of the high level REST client
from the server.

off (default): the warning is logged at WARN level. As before the
response is returned to the caller for further analysis. The caller
still can retrieve all warnings from the response.
on: the warning is logged at WARN level but the response is marked as
failed. The caller can retrieve all warnings from the ResponseException.

Fixes #33534

Minimal change that marks a rest request as failed if a deprecation warning has been issued by the server. This is valuable for internal tests and documentation snippets so that only non-deprecated code is used. This also minimizes possible divergence of the high level REST client from the server. `off` (default): the warning is logged at WARN level. As before the response is returned to the caller for further analysis. The caller still can retrieve all warnings from the response. `on`: the warning is logged at WARN level but the response is marked as failed. The caller can retrieve all warnings from the ResponseException.

jasontedor

I left a question. I'm super thankful that you picked this up and at a high-level it looks sane. We'll take a closer review after understanding why you opted for a system property instead of a more direct constructor parameter.

jasontedor · 2018-09-14T15:44:07Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

@@ -119,6 +120,7 @@
        this.failureListener = failureListener;
        this.pathPrefix = pathPrefix;
        this.nodeSelector = nodeSelector;
+        this.strictDeprecationMode = System.getProperty("es.deprecation.mode.strict", "off").equals("on");


I have one question and it's why you opted for a flag here instead of a constructor parameter?

Initially I also considered a ctor param, but changed my mind ;)

Remember: I originally started looking into this as the High level REST client integration tests did not fail once I deprecated a url parameter. So for me the strict mode is extremely useful for testing in general. Having a flag that a CI environment (elastic's or otherwise) can enforce is a kind of a must. And with the current approach the community can help without the need to understand and modify the way the HLRC initiates the low level client or the impressive test infrastructure for that matter;) This way the clean up can be done step by step: start cleaning the high level REST client (by fixing warning by warning), then flip the flag in the CI for the project; tackle the next project. And once all projects are cleaned, then flip it globally.
Basically I consider this a test-only parameter, which must not leak into production under any circumstances. Perhaps the name of the variable should be changed; any naming suggestions welcomed!

On the other hand the RestClient ctor already has a "few" positional parameters (currently 7). And if a tester or a CI needs to be able to set the mode as well, the discussion of whether the ctor setting or the system one should have the priority, is always a tricky one (I do not know what is the ES "common" approach in such a case). Also if it is implemented as a ctor param, then any subprojects will need to create a rest client with the appropriate setting and be cleaned in a single go.

Basically it boiled down to the fact that this approach gives the minimal viable solution, a proof of concept if you like. And it makes it easier for small isolated PRs. Changing it to a ctor param instead or having a combination of both is trivial to implement (as part of this PR or a separate one). But incredibly difficult to migrate to/adopt.

The idea behind having the builder for the rest client was so that we could add more parameters to ctor without bothering anyone. I admit there are a bunch though. I think I'd prefer to pass in a boolean or to make an interface that the client can implement to handle deprecations over a system property.

Thanks for explaining your reasoning @lipsill. I think that I would rather see a constructor parameter here. We can use the builder to reduce the pain on users. I am not concerned about our tests as we already fail test cases that have unexpected warning headers through our test infrastructure.

I am not concerned about our tests as we already fail test cases that have unexpected warning headers through our test infrastructure.

I think that only works on the YAML tests. If so this'd be the right hook to use to implement it for the other rest tests. That'd be a super useful followup if I'm right.

I don't think that warnings that come back over the low level rest client always get checked. I know we check them in the yaml test cases but I don't think that we push them into the thread context. So subclasses of ESRestTestCase that do not subclass ESClientYamlTestSuiteTestCase likely don't check for deprecations.

@nik9000 I am not sure that I understood you correctly. Is the idea here to always push any warnings from the REST tests to the thread context? So that the checks in the ESTestCase can be reused (the one that @jasontedor pointed out)? This way any internal test causing a warning fails.

Basically in test mode any warning is pushed to the thread context regardless of the strict mode setting? The strict mode setting is for the clients, so that they can check if they are using deprecated functionality.

Ah, you're right. Thanks for clarifying.

I honestly don't know what the right thing to do for tests is at this point, but I figure it is right to solve that problem after you introduce the strict mode. For the most part, tests should just use that.

Or, rather, for the most part, I expect tests not to get deprecations. So maybe, most tests should use strict mode and not worry about the thread context stuff. But we can think about that after this PR.

javanna · 2018-09-17T12:59:30Z

I think that we should also adapt the existing code that intended to print out the warnings here. I guess it does not work anymore given that the warnings format has changed and this code hasn't been updated. Would have been better to have an integration test maybe to test it with real warnings.

lipsill · 2018-09-17T15:28:02Z

As far as I can tell, this simply 'dumps' all Warning headers (regardless of the current format).
For example, in the rest layer tests log file the following line is printed

[2018-09-17T11:17:16,268][WARN ][o.e.c.RestClient         ] [[I/O dispatcher 2]] request [PUT http://127.0.0.1:62094/source/_split/copy-settings-target?master_timeout=10s&wait_for_active_shards=1&copy_settings=true&error_trace=true] returned 1 warnings: [299 Elasticsearch-7.0.0-alpha1-SNAPSHOT-76a2e89 "parameter [copy_settings] is deprecated and will be removed in 8.0.0" "Mon, 17 Sep 2018 15:17:15 GMT"]

containing the warning parameter [copy_settings] is deprecated and will be removed in 8.0.0 formatted by the current warning format .

I decided to leave the logging as is for now and not to parse the warning message from the http header. But it could be easily changed ;)

javanna · 2018-09-17T17:01:11Z

Ok cool I misunderstood where the regex is used. Glad to hear that the log lines are still printed. I thought it's related to this PR as the original issue mentions logging with strict mode disabled, so I thought that the current logging may need some adjustments, but maybe it's ok as-is.

elasticmachine · 2018-09-24T11:21:14Z

Pinging @elastic/es-core-infra

deprecation mode for the REST client

lipsill · 2018-09-25T19:06:56Z

@jasontedor @nik9000 I just pushed a commit that replaces the system property with a ctor parameter for the strict mode. Can you have a look if this is what you had in mind? Thanks!

nik9000

@lipsill, could you change a few small things? I left notes inline. Otherwise this is great!

In particular, I'd like to split how we integrate this into the testing framework as a follow up change. If you are interested in working on that as well as this that'd be wonderful. If not I'll have a look at it.

nik9000 · 2018-09-25T21:30:28Z

client/rest/src/main/java/org/elasticsearch/client/Response.java

@@ -96,6 +100,40 @@ public HttpEntity getEntity() {
        return response.getEntity();
    }

+    private static Pattern WARNING_HEADER_PATTERN = Pattern.compile(
+            "299 " + // warn code


Could you line all of these up vertically?

nik9000 · 2018-09-25T21:32:49Z

client/rest/src/main/java/org/elasticsearch/client/ResponseException.java

@@ -58,6 +58,10 @@ private static String buildMessage(Response response) throws IOException {
            response.getStatusLine().toString()
        );

+        if (response.hasWarnings()) {
+            message += "\n" + "Warnings: " + response.getWarnings();


nik9000 · 2018-09-25T21:37:22Z

test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

@@ -82,6 +82,7 @@
    public static final String CLIENT_RETRY_TIMEOUT = "client.retry.timeout";
    public static final String CLIENT_SOCKET_TIMEOUT = "client.socket.timeout";
    public static final String CLIENT_PATH_PREFIX = "client.path.prefix";
+    public static final String STRICT_DEPRECATION_MODE = "strict.deprecation.mode";


I'd save this for a follow up change. I suspect we'll want to control this some other way in tests. We might be able to get away with a method on ESRestTestCase that returns the default for it but I'm not sure. Either way, I think it should wait.

OK, I am removing it. My thinking here was to introduce the hook even if not used ;)
A method could be a way to go, I was following the way other tests are defining their settings (i.e. socket time out). But let's table this till the next PR ;)

nik9000 · 2018-09-25T21:39:46Z

client/rest/src/main/java/org/elasticsearch/client/Response.java

+                    "\""); // closing quote
+
+
+    public List<String> getWarnings() {


Could you add javadoc for this?

nik9000 · 2018-09-25T21:39:59Z

client/rest/src/main/java/org/elasticsearch/client/Response.java

+        return warnings;
+    }
+
+    public boolean hasWarnings() {


And could you add javadoc for this too?

nik9000

Looks good to me. I'll try and merge and backport this this afternoon.

nik9000 · 2018-09-26T18:45:52Z

@elasticmachine test this please

lipsill · 2018-09-26T20:04:31Z

@nik9000 Thanks for the review!

I just went ahead and merged master to see if it would help with the CI issue :

21:50:59 Execution failed for task ':distribution:bwc:next-bugfix-snapshot:buildBwcVersion'.
21:50:59 > Building bwc version didn't generate expected files ....

Can you trigger another CI?

jasontedor · 2018-09-27T01:55:30Z

@elasticmachine test this please

lipsill · 2018-09-27T11:29:28Z

I just had a look at the CI failure

04:15:45 FAILURE 0.04s J1 | MovAvgIT.testHoltWintersNotEnoughData <<< FAILURES!
04:15:45    > Throwable #1: junit.framework.AssertionFailedError: Expected exception SearchPhaseExecutionException but no exception was thrown

After bisect it seems like the issue is caused by ba3ceea. There is already an issue about it (#34098, #34046) and the test has already been muted (2b730d1, cb4cdf1).

As the failing test has been muted after the last time I merged, I will go ahead and merge again ;)

nik9000 · 2018-09-27T20:57:59Z

@elaticmachine, test this please. Again.

nik9000 · 2018-09-28T13:27:18Z

Thanks again @lipsill! I've merged and will backport to 6.x. Would you like to work on turning this on in the testing framework?

* master: Use more precise does S3 bucket exist method (elastic#34123) LLREST: Introduce a strict mode (elastic#33708) [CCR] Adjust list retryable errors (elastic#33985) Fix AggregationFactories.Builder equality and hash regarding order (elastic#34005) MINOR: Remove some deadcode in NodeEnv and Related (elastic#34133) Rest-Api-Spec: Correct spelling in filter_path description (elastic#33154) Core: Don't rely on java time for epoch seconds formatting (elastic#34086) Retry errors when fetching follower global checkpoint. (elastic#34019) Watcher: Reenable watcher stats REST tests (elastic#34107) Remove special-casing of Synonym filters in AnalysisRegistry (elastic#34034) Rename CCR APIs (elastic#34027) Fixed CCR stats api serialization issues and (elastic#33983) Support 'string'-style queries on metadata fields when reasonable. (elastic#34089) Logging: Drop Settings from security logger get calls (elastic#33940) SQL: Internal refactoring of operators as functions (elastic#34097)

Add tests for the Low Level REST Client's strict deprecation hangling. Relates to elastic#33708

lipsill · 2018-09-28T14:40:48Z

Would you like to work on turning this on in the testing framework?

Yes, I'd like to take a stab at it ;)

Introduces `RestClientBuilder#setStrictDeprecationMode` which defaults to false but when set to true, causes a rest request to fail if a deprecation warning header comes back in the response from Elasticsearch. This should be valueable to Elasticsearch's tests, especially those of the High Level REST Client where they will help catch divergence between the client and the server.

lipsill · 2018-10-05T20:04:44Z

@nik9000 I finally managed to get a passing build and just opened a PR to add the strict deprecated mode to the REST tests (#34338)
PS: a build takes ages... almost 5h on my hardware. Any tricks to speed it up?

Add tests for the Low Level REST Client's strict deprecation handling. Relates to #33708

nik9000 · 2018-10-06T16:48:20Z

PS: a build takes ages... almost 5h on my hardware. Any tricks to speed it up?

There are things, but it genuinely takes a long time because there is a lot going on. You could do ./gradlew integTest -xserver:integTest to just run the http tests, supplementing with ./gradlew precommit. That'll save some time but not a ton. I've mostly just thrown big machines at the problem.

#33708 introduced a strict deprecation mode that makes a REST request fail if there is a warning header in the response returned by Elasticsearch (usually a deprecation message signaling that a feature or a field has been deprecated). This change adds the strict deprecation mode into the REST integration tests, and makes the tests fail if a deprecated feature is used. Also any test using a deprecated feature has been modified to pass the build. The YAML integration tests already analyzed HTTP warnings so they do not use this mode, keeping their "expected vs actual" behavior.

colings86 · 2018-10-25T17:31:07Z

@nik9000 the backport is done here right?

nik9000 · 2018-10-25T18:18:20Z

Thanks for the ping @colings86! Yes, it is backported. Sorry for the mess!

Introduces `RestClientBuilder#setStrictDeprecationMode` which defaults to false but when set to true, causes a rest request to fail if a deprecation warning header comes back in the response from Elasticsearch. This should be valueable to Elasticsearch's tests, especially those of the High Level REST Client where they will help catch divergence between the client and the server.

Add tests for the Low Level REST Client's strict deprecation handling. Relates to #33708

#33708 introduced a strict deprecation mode that makes a REST request fail if there is a warning header in the response returned by Elasticsearch (usually a deprecation message signaling that a feature or a field has been deprecated). This change adds the strict deprecation mode into the REST integration tests, and makes the tests fail if a deprecated feature is used. Also any test using a deprecated feature has been modified to pass the build. The YAML integration tests already analyzed HTTP warnings so they do not use this mode, keeping their "expected vs actual" behavior.

jasontedor reviewed Sep 14, 2018

View reviewed changes

colings86 added the :Core/Java High Level REST Client label Sep 24, 2018

Use ctor param instead of a system property to specify strict

ef1c4ea

deprecation mode for the REST client

jasontedor requested a review from nik9000 September 25, 2018 19:09

nik9000 requested changes Sep 25, 2018

View reviewed changes

addressing reviewers remarks

38202c9

nik9000 approved these changes Sep 26, 2018

View reviewed changes

nik9000 added v7.0.0 v6.5.0 labels Sep 26, 2018

Merge remote-tracking branch 'origin/master' into rest_http_warnings

514ef27

Merge remote-tracking branch 'origin/master' into rest_http_warnings

486698e

nik9000 merged commit b3218fe into elastic:master Sep 28, 2018

nik9000 changed the title ~~REST client: introduce a strict mode es.deprecation.mode.strict~~ REST client: introduce a strict deprecation mode Sep 28, 2018

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Sep 28, 2018

LLRC: Test for warnings behavior

f82da1a

Add tests for the Low Level REST Client's strict deprecation hangling. Relates to elastic#33708

nik9000 added the backport pending label Sep 28, 2018

nik9000 mentioned this pull request Sep 28, 2018

LLRC: Test for warnings behavior #34143

Merged

lipsill deleted the rest_http_warnings branch September 28, 2018 14:40

lipsill mentioned this pull request Oct 5, 2018

[test] Introduce strict deprecation mode for REST tests #34338

Merged

nik9000 added a commit that referenced this pull request Oct 6, 2018

LLRC: Test for warnings behavior (#34143)

d905cc8

Add tests for the Low Level REST Client's strict deprecation handling. Relates to #33708

nik9000 added a commit that referenced this pull request Oct 6, 2018

LLRC: Test for warnings behavior (#34143)

fd03b2a

Add tests for the Low Level REST Client's strict deprecation handling. Relates to #33708

colings86 added the >enhancement label Oct 25, 2018

nik9000 removed the backport pending label Oct 25, 2018

kcm pushed a commit that referenced this pull request Oct 30, 2018

LLRC: Test for warnings behavior (#34143)

ab0c624

Add tests for the Low Level REST Client's strict deprecation handling. Relates to #33708

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

REST client: introduce a strict deprecation mode #33708

REST client: introduce a strict deprecation mode #33708

Conversation

lipsill commented Sep 14, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Sep 17, 2018

lipsill commented Sep 17, 2018

javanna commented Sep 17, 2018

elasticmachine commented Sep 24, 2018

lipsill commented Sep 25, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 commented Sep 26, 2018

lipsill commented Sep 26, 2018

jasontedor commented Sep 27, 2018

lipsill commented Sep 27, 2018

nik9000 commented Sep 27, 2018

nik9000 commented Sep 28, 2018

lipsill commented Sep 28, 2018

lipsill commented Oct 5, 2018

nik9000 commented Oct 6, 2018

colings86 commented Oct 25, 2018

nik9000 commented Oct 25, 2018