Start to uncouple deprecation handling from log4j by nik9000 · Pull Request #27955 · elastic/elasticsearch

nik9000 · 2017-12-22T02:59:31Z

We'd like to separate xcontent from Elasticsearch core so that the high level rest client can depend on it instead of Elasticsearch core. Unfortunately xcontent's deprecation handling uses Log4 and our logging utility classes explicitly. We don't want the high level rest client to require log4j2 or the logging utilities.

This proposes a solution: make the deprecation handling pluggable. In particular it proposes to change ParseField.match(String) into ParseField.match(String, DeprecationHandler). Core would contain the logging deprecation handler and other users could provide whatever they want.

Further it proposes that we add a member to XContentParser to hold the deprecation handler. This is somewhat strange as XContentParser doesn't directly call the DeprecationHandler. Instead, this PR proposes the idiom of using the DeprecationHandler on the XContentParser every time we call ParseField.match.

This end up forcing you to decide how to handle deprecated fields at parser creation time. This feels right to me because it is close to the code that receives that data being parsed in the first place. So you can look at the parser construction and say "oh, this is a user request, I should pitch deprecation warnings into the deprecation logger so they get sent back to the user" or "oh, this is a response from Elasticsearch in the high level rest client, I should ignore deprecated fields." Or not, I'm not sure what the high level rest client should do if it sees deprecated fields, but at least we have the option of changing it.

This does cause the problem of needing to specify the DeprecationHandler even in places where you are just doing low level parsing. This is a shame, but it seems worth it to be able to share parses across different parts of our code base.

Finally: this only does half the job. In a followup I'd have to replace all of the calls to ParseField.match(String) with ParseField.match(String, DeprecationHandler). I've done a few in this PR but doing them all would make the PR too large to review. It is already too large as is but I wanted to replace a few of the invocations so reviewers would have a taste of what the followup would look like.

This changes Elasticsearch's code so that anytime you make a new `XContentParser` you have to decide how to handle deprecationed fields by specifying a `DeprecationHandler`. There is no option to opt out. You have to think about where those deprecations should go. This goes through all constructions and uses one of three options: * `LoggingDeprecationHandler.INSTANCE` - logs deprecation warnings to the `DeprecationLogger` just like the code used to do for all such warnings. * `ParseField.UNSUPPORTED_OPERATION_DEPRECATION_HANDLER` - throws an `UnsupportedOperationException` if it receives any deprecation warnings. This is appropriate for places where Elasticsearch owns the data format because it is internal or for places where we never work with `ParseField`. * `ParseField.IGNORING_DEPRECATION_HANDLER` - totally ignores any deprecations. This is appropriate only when deprecation warnings are possible but the user shouldn't be told about them. So not many places. The way this deprecation handling is hooked up is that every call to `ParseField.match` should pass in a `DeprecationHandler`. By convention we always pass the one that this PR adds to `XContentParser`. This creates that nice "you decide what to do with deprecations when you build the parser" behavior. It would also be possible not to modify the parser at all and intead to make static references to the `DeprecationHandler` everywhere. While this is possible it really solve the problem of reusing parsers across separate projects with different deprecation requirements. Using the `DeprecationHandler` on the `XContentParser` does, even though it requires touching a lot of code.

I forgot how to mockito.

imotov

I left a few minor comments, but my primary concern is usage of LoggingDeprecationHandler in some requests and request builders. I think it undermines the initially stated goal of this change stating that "We don't want the high level rest client to require log4j2 or the logging utilities." Maybe we can pass the logger to the methods that are only used on the server or move these method out of the requests?

imotov · 2017-12-26T18:58:06Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

+        /*
+         * We use IGNORING_DEPRECATION_HANDLER because users of the high
+         * level rest client can't do anything about deprecated fields in
+         * the responses.


This comment gave me pause. I think it's somewhat misleading. Users can upgrade the version of the server, so they can do something about it. However, they might choose not to do that and this is perfectly fine. Maybe we can change this comment into something like "We use IGNORING_DEPRECATION_HANDLER because we are expected to work with deprecated fields while parsing responses from older version of elasticsearch and there is no need to notify users about every single occurrence of such field."

Makes sense to me.

I like that this makes us think about deprecated fields here.

imotov · 2017-12-26T19:46:45Z

core/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreFormat.java


    protected T read(BytesReference bytes) throws IOException {
-        try (XContentParser parser = XContentHelper.createParser(namedXContentRegistry, bytes)) {
+        // UNSUPPORTED_OPERATION_DEPRECATION_HANDLER is fine because we don't have deprecated fields in the blob store


This is not technically true, but I think we were not using index-version since 1.1.0, so we can probably remove it from the field definition.

Oh boy. I'm not sure what the deprecated field does then..... I can replace this with something else, but the LOGGING_DEPRECATION_HANDLER feels pretty weird here. We'd stuff that warning off in the ThreadLocal and if we're lucky we send it back to the user and they get confused because it points them to fields that they didn't send. If we're not lucky it sits in the ThreadLocal forever with no one to look at it.

I think we can either ignore it or fix BlobStoreIndexShardSnapshot to not have any deprecated fields.

imotov · 2017-12-26T19:55:20Z

core/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

            // EMPTY is safe here because we never call namedObject
-            try (XContentParser parser = xContent.createParser(NamedXContentRegistry.EMPTY, data.slice(from, nextMarker - from))) {
+            // TODO LoggingDeprecationHandler is probably not appropriate here because this is a request
+            // because LoggingDeprecationHandler is a server side thing but this is a request


It's actually used in both server and client content. So, maybe we need to have another version of this method that accepts logger as a parameter and pass LoggingDeprecationHandler on the server side and IGNORING_DEPRECATION_HANDLER on the client.

imotov · 2017-12-26T19:59:17Z

core/src/main/java/org/elasticsearch/action/admin/indices/create/CreateIndexRequest.java

    public CreateIndexRequest aliases(BytesReference source) {
        // EMPTY is safe here because we never call namedObject
-        try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, source)) {
+        // TODO LoggingDeprecationHandler probably should be visible to a request


That wouldn't this kind of kill the whole idea of separating log4j from high level rest clients?

I left it like this because I didn't want to change too many things in this PR. I'd love to change how this works but that is technically a breaking change for the transport client and the high level rest client and I didn't want to bury it with all the internal changes in this PR. Basically I think we should change this but I didn't want to do it now.

I would be fine with dealing with this in a separate PR.

imotov · 2017-12-26T20:02:43Z

core/src/test/java/org/elasticsearch/cluster/metadata/IndexTemplateMetaDataTests.java

 import java.util.Base64;
 import java.util.Collections;

+import static java.util.Collections.emptyList;


Not needed.

imotov · 2017-12-26T20:06:10Z

core/src/test/java/org/elasticsearch/common/ParseFieldTests.java


+import org.elasticsearch.common.ParseField.DeprecationHandler;
 import org.elasticsearch.test.ESTestCase;
+import org.mockito.ArgumentCaptor;


imotov · 2017-12-26T20:06:23Z

core/src/test/java/org/elasticsearch/common/ParseFieldTests.java

 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.CoreMatchers.sameInstance;
 import static org.hamcrest.collection.IsArrayContainingInAnyOrder.arrayContainingInAnyOrder;
+import static org.mockito.Mockito.eq;


Yeah. You need to use eq if you are capturing arguments. You can't just send in an object.

And I'll remove it.

imotov · 2017-12-26T20:12:08Z

core/src/main/java/org/elasticsearch/common/settings/Settings.java

         */
        public Builder loadFromSource(String source, XContentType xContentType) {
-            try (XContentParser parser =  XContentFactory.xContent(xContentType).createParser(NamedXContentRegistry.EMPTY, source)) {
+            // UNSUPPORTED_OPERATION_DEPRECATION_HANDLER is fine here because we do not have deprecated fields


Maybe change this to "... because we don't use ParseField"?

nik9000 · 2018-01-30T20:49:26Z

Replaced by #28449.

nik9000 added 8 commits December 21, 2017 09:31

WIP

38fc94d

WAP

01527c2

Too much

585af19

More constructing

c6824e8

Fix test

a730054

Fix other test

bb5dc2a

I forgot how to mockito.

Undo silly change

910d276

nik9000 added :Internal v6.2.0 v7.0.0 labels Dec 22, 2017

nik9000 requested review from dakrone, imotov and jasontedor December 22, 2017 02:59

nik9000 added the >breaking-java label Dec 22, 2017

imotov reviewed Dec 26, 2017

View reviewed changes

colings86 added v6.3.0 and removed v6.2.0 labels Jan 22, 2018

nik9000 removed the review label Jan 30, 2018

nik9000 closed this Jan 30, 2018

nik9000 mentioned this pull request Jan 30, 2018

XContent: Factor deprecation handling into callback #28449

Merged

nik9000 removed v6.3.0 v7.0.0 labels Jan 30, 2018

elasticmachine mentioned this pull request Apr 25, 2018

SQL: Make the JDBC driver a single JAR again #29856

Closed

Conversation

nik9000 commented Dec 22, 2017

Uh oh!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 Dec 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nik9000 Dec 26, 2017 •

edited

Loading