You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "bmscomp (via GitHub)" <gi...@apache.org> on 2023/05/02 22:38:19 UTC
[GitHub] [kafka] bmscomp opened a new pull request, #13662: MINOR: Upgrade Jackson dependencies to version to 2.15.0
bmscomp opened a new pull request, #13662:
URL: https://github.com/apache/kafka/pull/13662
Upgrade Jackson dependencies to version `2.15.0`
- Unify the Jackson and Jackson Data bind dependencies to the latest stable version 2.15.0
- 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: KAFKA-15208: Upgrade Jackson dependencies to version 2.15.2
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1641878105
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0 [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1862865913
None of the failures in the latest build seem related, merging to trunk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0 [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1862875333
Backported to 3.7: https://github.com/apache/kafka/commit/62c7e8b13bff807c652a64b462ff370b66b94319
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.15.3 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1762253746
The pull request is updated to take advantages of version 2.15.3 of `jackson databind ` that aims to fix much more issues and bring more enhancements, as we can mention :
[#3968](https://github.com/FasterXML/jackson-databind/issues/3968): Records with additional constructors failed to deserialize
[#4121](https://github.com/FasterXML/jackson-databind/issues/4121): Preserve the original component type in merging to an array
[#426](https://github.com/FasterXML/jackson-dataformats-text/issues/426): Update to SnakeYAML 2.1
Nothing changed related to the limitations make on this version of the field serialisation of json to solve security issues
To know more about this version please refer to the release notes https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.15.3
@divijvaidya Please can you take a look at this update
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13662: KAFKA-15208: Upgrade Jackson dependencies to version 2.15.2
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1641713724
`FileBasedStateStore` and `QuorumState` are not the only place that uses jackson. Did you check usage of `objectmapper`, `Json.scala` etc. for the limits question?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1546140859
@ijuma thank you for the comment, I'll update this pull PR and make another one for clean-ups
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.15.3 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1847567651
@mimaison I already upgraded to version 2.16.0 and updated the pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0 [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1850229092
@divijvaidya Do you have any further comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.15.3 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1843889093
@mimaison Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.15.3 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13662:
URL: https://github.com/apache/kafka/pull/13662#discussion_r1420566217
##########
LICENSE-binary:
##########
@@ -216,16 +216,16 @@ commons-lang3-3.8.1
commons-logging-1.2
commons-validator-1.7
error_prone_annotations-2.10.0
-jackson-annotations-2.13.5
-jackson-core-2.13.5
-jackson-databind-2.13.5
-jackson-dataformat-csv-2.13.5
-jackson-datatype-jdk8-2.13.5
-jackson-jaxrs-base-2.13.5
-jackson-jaxrs-json-provider-2.13.5
-jackson-module-jaxb-annotations-2.13.5
-jackson-module-scala_2.13-2.13.5
-jackson-module-scala_2.12-2.13.5
+jackson-annotations-2.15.3
Review Comment:
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1583266979
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.15.3 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13662:
URL: https://github.com/apache/kafka/pull/13662#discussion_r1420566217
##########
LICENSE-binary:
##########
@@ -216,16 +216,16 @@ commons-lang3-3.8.1
commons-logging-1.2
commons-validator-1.7
error_prone_annotations-2.10.0
-jackson-annotations-2.13.5
-jackson-core-2.13.5
-jackson-databind-2.13.5
-jackson-dataformat-csv-2.13.5
-jackson-datatype-jdk8-2.13.5
-jackson-jaxrs-base-2.13.5
-jackson-jaxrs-json-provider-2.13.5
-jackson-module-jaxb-annotations-2.13.5
-jackson-module-scala_2.13-2.13.5
-jackson-module-scala_2.12-2.13.5
+jackson-annotations-2.15.3
Review Comment:
I know about this release I think it worth it to upgrade to it, I'll give it a try
##########
LICENSE-binary:
##########
@@ -216,16 +216,16 @@ commons-lang3-3.8.1
commons-logging-1.2
commons-validator-1.7
error_prone_annotations-2.10.0
-jackson-annotations-2.13.5
-jackson-core-2.13.5
-jackson-databind-2.13.5
-jackson-dataformat-csv-2.13.5
-jackson-datatype-jdk8-2.13.5
-jackson-jaxrs-base-2.13.5
-jackson-jaxrs-json-provider-2.13.5
-jackson-module-jaxb-annotations-2.13.5
-jackson-module-scala_2.13-2.13.5
-jackson-module-scala_2.12-2.13.5
+jackson-annotations-2.15.3
Review Comment:
@mimaison I know about this release I think it worth it to upgrade to it, I'll give it a try
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13662:
URL: https://github.com/apache/kafka/pull/13662#discussion_r1428518449
##########
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java:
##########
@@ -143,7 +143,7 @@ public class JsonConverter implements Converter, HeaderConverter, Versioned {
// names specified in the field
private static final HashMap<String, LogicalTypeConverter> LOGICAL_CONVERTERS = new HashMap<>();
- private static final JsonNodeFactory JSON_NODE_FACTORY = JsonNodeFactory.withExactBigDecimals(true);
+ private static final JsonNodeFactory JSON_NODE_FACTORY = new JsonNodeFactory(true);
Review Comment:
@mimaison Sorry I missed answering this point, the change is made because the static method is deprecated
```
@Deprecated
public static JsonNodeFactory withExactBigDecimals(boolean bigDecimalExact) {
return new JsonNodeFactory(bigDecimalExact);
}
```
and the best available replacement for it is the the constructor
```
public JsonNodeFactory(boolean bigDecimalExact) {
this._cfgBigDecimalExact = bigDecimalExact;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1580008259
@bmscomp , could you respond to @divijvaidya 's comments above?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.15.3 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1842612531
@mimaison Thanks for the review I'll to this asap by the end of day
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1535937778
@divijvaidya I get your point, thanks so much for those explanations
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1554532930
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: KAFKA-15208: Upgrade Jackson dependencies to version 2.15.2
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1648586221
@divijvaidya after analysing the regression regarding Kafka code base, it seems that the regression is related to records, and in the current version of Kafka records are not introduced yet, then we are not concerned by this regression
But it's more wise to wait the next release of jackson-databind
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.2
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1637174843
@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();
System.out.println(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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13662:
URL: https://github.com/apache/kafka/pull/13662#discussion_r1428518449
##########
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java:
##########
@@ -143,7 +143,7 @@ public class JsonConverter implements Converter, HeaderConverter, Versioned {
// names specified in the field
private static final HashMap<String, LogicalTypeConverter> LOGICAL_CONVERTERS = new HashMap<>();
- private static final JsonNodeFactory JSON_NODE_FACTORY = JsonNodeFactory.withExactBigDecimals(true);
+ private static final JsonNodeFactory JSON_NODE_FACTORY = new JsonNodeFactory(true);
Review Comment:
@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;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1535880466
@bmscomp this is a nice PR and I support it. I opened the other one ( https://github.com/apache/kafka/pull/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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1546745756
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.2
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1640035902
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0 [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #13662:
URL: https://github.com/apache/kafka/pull/13662
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1535524442
@divijvaidya this pull request targets an update to version 2.15.0, please can you tell me better about updating not only minor versions ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13662: KAFKA-15208: Upgrade Jackson dependencies to version 2.15.2
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1647835092
2.15.0 has a regression: https://github.com/FasterXML/jackson-databind/issues/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: KAFKA-15208: Upgrade Jackson dependencies to version 2.15.2
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1647865421
The regression may be solved on coming version 2.15.3, https://github.com/FasterXML/jackson-databind/pull/3969
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1559122983
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.15.3 [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #13662:
URL: https://github.com/apache/kafka/pull/13662#discussion_r1420500562
##########
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java:
##########
@@ -143,7 +143,7 @@ public class JsonConverter implements Converter, HeaderConverter, Versioned {
// names specified in the field
private static final HashMap<String, LogicalTypeConverter> LOGICAL_CONVERTERS = new HashMap<>();
- private static final JsonNodeFactory JSON_NODE_FACTORY = JsonNodeFactory.withExactBigDecimals(true);
+ private static final JsonNodeFactory JSON_NODE_FACTORY = new JsonNodeFactory(true);
Review Comment:
Can you clarify why we're making this changes?
##########
LICENSE-binary:
##########
@@ -216,16 +216,16 @@ commons-lang3-3.8.1
commons-logging-1.2
commons-validator-1.7
error_prone_annotations-2.10.0
-jackson-annotations-2.13.5
-jackson-core-2.13.5
-jackson-databind-2.13.5
-jackson-dataformat-csv-2.13.5
-jackson-datatype-jdk8-2.13.5
-jackson-jaxrs-base-2.13.5
-jackson-jaxrs-json-provider-2.13.5
-jackson-module-jaxb-annotations-2.13.5
-jackson-module-scala_2.13-2.13.5
-jackson-module-scala_2.12-2.13.5
+jackson-annotations-2.15.3
Review Comment:
There's even a 2.16.0 release now. Is there a reason you picked 2.15.3?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-15208: Upgrade Jackson dependencies to version 2.16.0 [kafka]
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on code in PR #13662:
URL: https://github.com/apache/kafka/pull/13662#discussion_r1428518449
##########
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java:
##########
@@ -143,7 +143,7 @@ public class JsonConverter implements Converter, HeaderConverter, Versioned {
// names specified in the field
private static final HashMap<String, LogicalTypeConverter> LOGICAL_CONVERTERS = new HashMap<>();
- private static final JsonNodeFactory JSON_NODE_FACTORY = JsonNodeFactory.withExactBigDecimals(true);
+ private static final JsonNodeFactory JSON_NODE_FACTORY = new JsonNodeFactory(true);
Review Comment:
@mimaison Sorry I missed answering this point, the change is made because the static method is deprecated
```
@Deprecated
public static JsonNodeFactory withExactBigDecimals(boolean bigDecimalExact) {
return new JsonNodeFactory(bigDecimalExact);
}
```
and the best available replacement for it is the the constructor
```
/**
* 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);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.0
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1545692385
It would be better to have a separate PR for clean-ups unrelated to the Jackson dependency update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.2
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1637051977
@divijvaidya License file updated to mention the correct jackson version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.2
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1637198127
@divijvaidya Yes I agree with you this is not a minor change, we need to create a JIRA ticket for that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bmscomp commented on pull request #13662: MINOR: Upgrade Jackson dependencies to version 2.15.2
Posted by "bmscomp (via GitHub)" <gi...@apache.org>.
bmscomp commented on PR #13662:
URL: https://github.com/apache/kafka/pull/13662#issuecomment-1640146096
@divijvaidya I am pretty sure the introduced 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org