Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start to uncouple deprecation handling from log4j #27955

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
3 participants
@nik9000
Copy link
Contributor

commented Dec 22, 2017

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.

nik9000 added some commits Dec 21, 2017

WIP
WAP
Start to uncouple deprecation handling from logging
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.
Fix other test
I forgot how to mockito.
@imotov
Copy link
Member

left a comment

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?

/*
* 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 has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

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."

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 26, 2017

Author Contributor

Makes sense to me.

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 26, 2017

Author Contributor

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

@@ -109,7 +110,9 @@ protected String blobName(String name) {
}

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 comment has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

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.

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 26, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@imotov

imotov Dec 27, 2017

Member

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

@@ -304,7 +305,10 @@ public BulkRequest add(BytesReference data, @Nullable String defaultIndex, @Null

// now parse the action
// 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

This comment has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

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.

@@ -318,7 +320,10 @@ public CreateIndexRequest aliases(String source) {
*/
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

This comment has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

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

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 26, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@imotov

imotov Dec 27, 2017

Member

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

@@ -39,12 +36,12 @@
import java.util.Base64;
import java.util.Collections;

import static java.util.Collections.emptyList;

This comment has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

Not needed.

import org.elasticsearch.test.ESTestCase;
import org.mockito.ArgumentCaptor;

This comment has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

Leftovers?

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 26, 2017

Author Contributor

Yes.


import static org.hamcrest.CoreMatchers.is;
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;

This comment has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

Leftovers?

This comment has been minimized.

Copy link
@nik9000

nik9000 Dec 26, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@nik9000

nik9000 Jan 3, 2018

Author Contributor

And I'll remove it.

@@ -1114,7 +1115,9 @@ private void processLegacyLists(Map<String, Object> map) {
* Loads settings from the actual string content that represents them using {@link #fromXContent(XContentParser)}
*/
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

This comment has been minimized.

Copy link
@imotov

imotov Dec 26, 2017

Member

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

@nik9000

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

Replaced by #28449.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.