KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0

[bmscomp]

Upgrade Jackson dependencies to version 2.16.0

• Unify the Jackson and Jackson Data bind dependencies and upgrade to the latest stable version 2.15.2
• Remove deprecation call of JsonNodeFactory.withExactBigDecimals
• Fix some typos in comments and documentation

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[bmscomp]

@divijvaidya this pull request targets an update to version 2.15.0, please can you tell me better about updating not only minor versions ?

[divijvaidya]

@bmscomp this is a nice PR and I support it. I opened the other one ( #13673 ) for minor versions upgrades because minor versions have a lower risk and require less detailed review for reviewers. My ideal situation would be if we can merge the minor version updates into the upcoming 3.5 (it's safe because of minor upgrades) and then target key major version upgrades of dependencies such as this PR for 3.6. Nevertheless, let's hear back from what other folks in the community have to say about this.

(If the minor version PR doesn't make it to 3.5, I will remove jackson changes from it so that jackson can be addressed by this PR).

[bmscomp]

@divijvaidya I get your point, thanks so much for those explanations

[ijuma]

It would be better to have a separate PR for clean-ups unrelated to the Jackson dependency update.

[bmscomp]

@ijuma thank you for the comment, I'll update this pull PR and make another one for clean-ups

[bmscomp]

@ijuma the PR is updated , I managed to keep only modifications related to the upgrade of Jackson version to 2.15.0, I'll make another separate PR related to clean-ups

[divijvaidya]

Breaking changes between 2.13.x and 2.15.x:

1. @JsonIgnore gets more precedence than @JsonProperty in 2.15.x in case both annotations are assigned to a property. Prior to 2.14.x, it was the opposite precedence. See: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.14#observedreported-changes
   This should not impact Kafka since I couldn't find usages of @JsonIgnore in the code base.

2. ProcessingLimits adds limitation to length of different tokens. Prior to 2.15.x, there was no limits. See: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15#processing-limits
   Verified that FileBasedStateStore is not impacted by the new limits.

3. Modified handling of datetime with zone information in it. See: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15#changes-behavior-other

@bmscomp could you please go through the usage of Jackson in Kafka code base and add here (similar to above) on why the new processing limits won't impact existing code (or comment if we want to increase the limits from default values). Same for ZoneId handling. Given the number of breaking changes, I think we should also create a JIRA for this (and associate the PR). This is not a minor change!

Thanks!

[divijvaidya]

I found today that there is another change that needs to make it in this PR. We need to update the LICENSE file with the correct version at https://github.com/apache/kafka/blob/trunk/LICENSE-binary#L211

[showuon]

@bmscomp , could you respond to @divijvaidya 's comments above? Thanks.

[bmscomp]

@showuon Yes I'll do ,Thanks so much @divijvaidya for reviews, I am back from holidays :) , I'll continue working on this topic, I'll check all comments one by one and try to bring the best answer possible that I'lll be able to do

Thanks again for all reviews

[bmscomp]

@divijvaidya License file updated to mention the correct jackson version

[bmscomp]

@divijvaidya The third point you mentioned
Modified handling of datetime with zone information in it. See: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15#changes-behavior-other does not affect the existing code base, it solves an issue, it's a correction of an existing bug in previous jackson version that prevent producing that kind of bugs

ZonedDateTime now = ZonedDateTime.now();
assertTrue(now.withZoneSameInstant(ZoneOffset.UTC)
        .equals(now.withZoneSameInstant(ZoneId.of("UTC"))));

I use to think that we need to write tests on json Serialization and Deserialization of zoned datetime values

[bmscomp]

@divijvaidya Yes I agree with you this is not a minor change, we need to create a JIRA ticket for that

[divijvaidya]

Hey @bmscomp
I am waiting for an answer to the following. Please tag me again when this is ready for review.

@bmscomp could you please go through the usage of Jackson in Kafka code base and add here (similar to above) on why the new processing limits won't impact existing code (or comment if we want to increase the limits from default values).

[bmscomp]

@divijvaidya I am pretty sure the introduced default values for limits wont affect the existing processing capacity, FileBasedStateStore is mainly used to store Quorum state data, and I use to think there is no need to raise the limits from default values

Since in this version:

• Maximum number token lengths is set to 1000 as maximum number of bytes or chars that we will parse in a number
• Maximum String value length is raised to 20_000_000 (20 million) input units bytes/chars depending on input source) in 2.15.1 and the initial value in version 2.15.0 was 5_000_000 (5 million)
• Maximum Input nesting depth is set up to 1000 levels, and this is too huge QuorumStateData does not have as much as levels

[divijvaidya]

FileBasedStateStore and QuorumState are not the only place that uses jackson. Did you check usage of objectmapper, Json.scala etc. for the limits question?

[bmscomp]

@divijvaidya For the internal usage of Jackson, I can say with much confidence that the limits are far more then what is needed to run Kafka, for it's usage the message.max.bytes in Kafka it's by default 1MB, and this is also far below limits for message transformation Serialization and Deserialization

The limit not put in the message size but the attribute value size and for example String values limit is raised to 20 megabytes, and this is already huge,

Do not think it's necessary to raise those default values

[divijvaidya]

2.15.0 has a regression: FasterXML/jackson-databind#3968

We may want to directly want to jump to that to avoid the regression. Let's see what is the latest available version by code freeze date of 3.6 and we will merge that in.

[bmscomp]

@mimaison Thanks for the review I'll to this asap by the end of day

[bmscomp]

@mimaison Done!

[mimaison]

Thanks for the updates. I left a couple of questions

[mimaison]

Can you clarify why we're making this changes?

[bmscomp]

@mimaison Sorry I missed answering this point, the change is made because the static method is deprecated

    /**
     * Return a factory instance with the desired behavior for BigDecimals
     * <p>See {@link #JsonNodeFactory(boolean)} for a full description.</p>
     *
     * @param bigDecimalExact If {code true} DISABLE normalization of {@link BigDecimal} values;
     *    if {code false} ENABLE normalization
     * @return a factory instance with specified configuration
     *
     * @deprecated Use {@link com.fasterxml.jackson.databind.cfg.JsonNodeFeature#STRIP_TRAILING_BIGDECIMAL_ZEROES}
     *   instead for configuring behavior.
     */
    @Deprecated
    public static JsonNodeFactory withExactBigDecimals(boolean bigDecimalExact)
    {
        return new JsonNodeFactory(bigDecimalExact);
    }
 

and the best available replacement for it is the the constructor

    /**
     * @param bigDecimalExact see Class description on "BigDecimal normalization"
     *
     * @see BigDecimal
     */
    public JsonNodeFactory(boolean bigDecimalExact)
    {
        _cfgBigDecimalExact = bigDecimalExact;
    }

[mimaison]

There's even a 2.16.0 release now. Is there a reason you picked 2.15.3?

[bmscomp]

@mimaison I know about this release I think it worth it to upgrade to it, I'll give it a try, but suppose the 2.15.3 is far stable till now

[bmscomp]

@mimaison I already upgraded to version 2.16.0 and updated the pull request

[mimaison]

LGTM, thanks for the PR

[mimaison]

@divijvaidya Do you have any further comments?

[divijvaidya]

Some additional things that I verified:

• JDK version supported is 8 and above.
• scala versions 2.11 and above are supported.
• the limits added in this version are sufficiently large for Kafka use cases. 1000 levels of nesting, 50k chars in property name and unlimited maximum content length.
• this version upgrades dependency PCollection to 4.x, Kafka already uses 4.1 version for PCollection

Overall change looks good to me. I am waiting for CI to succeed.

[mimaison]

None of the failures in the latest build seem related, merging to trunk

[mimaison]

Backported to 3.7: 62c7e8b