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

Update jackson to version 2.8.1 (on 2.4 branch) #20011

Merged
merged 1 commit into from Aug 19, 2016

Conversation

Projects
None yet
3 participants
@tlrx
Member

tlrx commented Aug 16, 2016

Backport of #18939 on 2.4 branch.

@tlrx tlrx changed the title from Update jackson 2.8.1 on 2.4 to Update jackson to version 2.8.1 (on 2.4 branch) Aug 16, 2016

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Aug 16, 2016

Member

@tlrx I think you opened this on the wrong branch? Github just added the ability to change the branch a PR is against, so maybe that would help?

Member

dakrone commented Aug 16, 2016

@tlrx I think you opened this on the wrong branch? Github just added the ability to change the branch a PR is against, so maybe that would help?

@tlrx tlrx changed the base branch from master to 2.4 Aug 17, 2016

@tlrx

This comment has been minimized.

Show comment
Hide comment
@tlrx

tlrx Aug 17, 2016

Member

Thanks @dakrone. I did this just before going to sleep, I should have wait until the next morning :/

@elasticmachine retest this please

Member

tlrx commented Aug 17, 2016

Thanks @dakrone. I did this just before going to sleep, I should have wait until the next morning :/

@elasticmachine retest this please

@tlrx

This comment has been minimized.

Show comment
Hide comment
Member

tlrx commented Aug 18, 2016

base.getOutputContext().writeValue();
JsonStreamContext context = base.getOutputContext();
assert (context instanceof JsonWriteContext) : "Expected an instance of JsonWriteContext but was: " + context.getClass();
((JsonWriteContext) context).writeValue();

This comment has been minimized.

@abeyad

abeyad Aug 18, 2016

Contributor

why do we need to use the casted form of the object here? was it just to add the assert?

@abeyad

abeyad Aug 18, 2016

Contributor

why do we need to use the casted form of the object here? was it just to add the assert?

This comment has been minimized.

@tlrx

tlrx Aug 18, 2016

Member

Not sure to get your question, but we need to ensure that we're using a JsonWriteContext because we need to call writeValue() to produce valid content (it updates some index in the context).

@tlrx

tlrx Aug 18, 2016

Member

Not sure to get your question, but we need to ensure that we're using a JsonWriteContext because we need to call writeValue() to produce valid content (it updates some index in the context).

This comment has been minimized.

@abeyad

abeyad Aug 18, 2016

Contributor

what i meant was, before we had base.getOutputContext().writeValue(), which will call the writeValue() method on whatever sub-class the base.getOutputContext() resolves to, so do we really need to do the cast? Wouldn't base.getOutputContext().writeValue() call the writeValue() of JsonWriteContext if base.getOutputContext() returns an instance of JsonWriteContext?

@abeyad

abeyad Aug 18, 2016

Contributor

what i meant was, before we had base.getOutputContext().writeValue(), which will call the writeValue() method on whatever sub-class the base.getOutputContext() resolves to, so do we really need to do the cast? Wouldn't base.getOutputContext().writeValue() call the writeValue() of JsonWriteContext if base.getOutputContext() returns an instance of JsonWriteContext?

This comment has been minimized.

@abeyad

abeyad Aug 18, 2016

Contributor

oh i see, the JsonStreamContext now doesn't implement writeValue().

@abeyad

abeyad Aug 18, 2016

Contributor

oh i see, the JsonStreamContext now doesn't implement writeValue().

This comment has been minimized.

@tlrx

tlrx Aug 18, 2016

Member

Yes :)

@tlrx

tlrx Aug 18, 2016

Member

Yes :)

@abeyad

View changes

Show outdated Hide outdated core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java
@abeyad

View changes

Show outdated Hide outdated core/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java
@abeyad

View changes

Show outdated Hide outdated ...arch/search/aggregations/bucket/SignificantTermsSignificanceScoreIT.java
@abeyad

This comment has been minimized.

Show comment
Hide comment
@abeyad

abeyad Aug 18, 2016

Contributor

@tlrx LGTM, just left a minor comment on using Strings.toString(XContentBuilder, true) to serialize. Otherwise, feel free to merge when ready.

Contributor

abeyad commented Aug 18, 2016

@tlrx LGTM, just left a minor comment on using Strings.toString(XContentBuilder, true) to serialize. Otherwise, feel free to merge when ready.

@tlrx

This comment has been minimized.

Show comment
Hide comment
@tlrx

tlrx Aug 18, 2016

Member

Thanks @abeyad ! I rebased and updated the code and will merge once tests are fine.

Member

tlrx commented Aug 18, 2016

Thanks @abeyad ! I rebased and updated the code and will merge once tests are fine.

@tlrx tlrx merged commit 289309a into elastic:2.4 Aug 19, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@tlrx tlrx deleted the tlrx:update-jackson-2.8.1-on-2.4 branch Aug 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment