Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1869045953 > Do you have any estimations? 3.8 is usually expected to release around ~Apr-May timeframe. You can find more details about timelines of our releases here: https://cwiki.apache.org/confluence/display/KAFKA/Time+Based+Release+Plan -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1869012147 > We don't usually backport performance fixes to patch versions. (With the usual caveat that this is not set in stone and is open to discussion). Our motivation is to backport only critical changes in the patch version so that we don't accidentally introduce a regression. It's fair. So, normally it's expected to be in 3.8 release. Do you have any estimations? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1868993157 > Do you think there can be a chance for 3.7.1? Is there any schedule? We don't usually backport performance fixes to patch versions. (With the usual caveat that this is not set in stone and is open to discussion). Our motivation is to backport only critical changes in the patch version so that we don't accidentally introduce a regression. We don't current have a date for 3.7.1 but going by last few releases, it is highly likely that we will have a patch 3.7.1 release. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1868612393 @divijvaidya I agree this is not a critical issue. Do you think there can be a chance for 3.7.1? Is there any schedule? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1868606486 @mfvitale No, we missed that train. We are currently past the code freeze for 3.7 (see: https://cwiki.apache.org/confluence/display/KAFKA/Release+Plan+3.7.0). Given this is a performance improvement feature and not a major bug fix, I am currently not in favour of pushing this to be included as an exception to 3.7. However, if you think that this should go in 3.7, we can discuss about that. What do you think? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1868596329 @divijvaidya do you know if it will be in 3.7? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya merged PR #14992: URL: https://github.com/apache/kafka/pull/14992 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1868590920 I have verified that failing tests in the CI are known to be flaky or/and are failing for other PRs as well. - org.apache.kafka.streams.integration.StandbyTaskEOSMultiRebalanceIntegrationTest.shouldHonorEOSWhenUsingCachingAndStandbyReplicas has been failing with similar error message in rest of the PRs as well. - kafka.server.BrokerRegistrationRequestTest.testRegisterZkWith33Controller failing for other PRs as well -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1867484294 > haha! I like your commit message "code beauty" :) > I see that this is your first change to Apache Kafka. Congratulations and welcome! Yes! Thanks! > As a next step, I will wait for CI to be stable before I merge this change in. Perfect! -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1867448606 @divijvaidya Fixed! -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1867441468 @mfvitale one last comment about fixing indentation, otherwise we should be good to ship! -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya commented on code in PR #14992:
URL: https://github.com/apache/kafka/pull/14992#discussion_r1434876077
##
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java:
##
@@ -235,18 +235,31 @@ public Object toConnect(final Schema schema, final
JsonNode value) {
private final JsonDeserializer deserializer;
public JsonConverter() {
+this(true);
+}
+
+/**
+ * Creates a JsonConvert initializing serializer and deserializer.
+ *
+ * @param enableModules permits to enable/disable the registration of
additional Jackson modules.
+ *
+ * NOTE: This is visible only for testing
+ */
+public JsonConverter(boolean enableModules) {
serializer = new JsonSerializer(
mkSet(),
-JSON_NODE_FACTORY
+JSON_NODE_FACTORY,
+enableModules
);
deserializer = new JsonDeserializer(
mkSet(
-// this ensures that the JsonDeserializer maintains full
precision on
-// floating point numbers that cannot fit into float64
-DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS
+// this ensures that the JsonDeserializer maintains full
precision on
Review Comment:
please follow original indentation.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1866538380 > Thank you for this PR. I have left some minor comments. > > Also, please add a JIRA targeting 4.0 (where we will deprecate JDK 8) to replace afterburn with blackbird. @divijvaidya https://issues.apache.org/jira/browse/KAFKA-16041 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on code in PR #14992:
URL: https://github.com/apache/kafka/pull/14992#discussion_r1434225385
##
build.gradle:
##
@@ -940,6 +940,7 @@ project(':core') {
implementation libs.jacksonModuleScala
implementation libs.jacksonDataformatCsv
implementation libs.jacksonJDK8Datatypes
+implementation libs.jacksonAfterburner
Review Comment:
I have been misled by other Jackson dependencies. Removed.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
divijvaidya commented on code in PR #14992:
URL: https://github.com/apache/kafka/pull/14992#discussion_r1434120692
##
build.gradle:
##
@@ -940,6 +940,7 @@ project(':core') {
implementation libs.jacksonModuleScala
implementation libs.jacksonDataformatCsv
implementation libs.jacksonJDK8Datatypes
+implementation libs.jacksonAfterburner
Review Comment:
Please help me understand why did we add this dependency in clients?
##
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java:
##
@@ -235,18 +235,24 @@ public Object toConnect(final Schema schema, final
JsonNode value) {
private final JsonDeserializer deserializer;
public JsonConverter() {
+this(true);
+}
+
+public JsonConverter(boolean enableModules) {
serializer = new JsonSerializer(
-mkSet(),
-JSON_NODE_FACTORY
+mkSet(),
Review Comment:
nit
please maintain original indentation
##
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java:
##
@@ -235,18 +235,24 @@ public Object toConnect(final Schema schema, final
JsonNode value) {
private final JsonDeserializer deserializer;
public JsonConverter() {
+this(true);
+}
+
+public JsonConverter(boolean enableModules) {
Review Comment:
please add a javadoc comment specifying that this ctor is visible only for
benchmarking. we add similar comments when we expose a new ctor/method for
testing.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1864230355 > Could you write a small jmh benchmark to demonstrate the performance difference? For example, see #13776 Done! These are some results ```text Benchmark (afterBurnModule)Mode CntScoreError Units JsonConverterBenchmark.deserialize true sample 138716917997.199 ± 17.127 ns/op JsonConverterBenchmark.deserialize:p0.00 true sample 16384.000 ns/op JsonConverterBenchmark.deserialize:p0.50 true sample 17408.000 ns/op JsonConverterBenchmark.deserialize:p0.90 true sample 18304.000 ns/op JsonConverterBenchmark.deserialize:p0.95 true sample 20672.000 ns/op JsonConverterBenchmark.deserialize:p0.99 true sample 30720.000 ns/op JsonConverterBenchmark.deserialize:p0.999true sample 40757.120 ns/op JsonConverterBenchmark.deserialize:p0. true sample 423056.896 ns/op JsonConverterBenchmark.deserialize:p1.00 true sample 924672.000 ns/op JsonConverterBenchmark.deserialize false sample 130360019164.929 ± 21.371 ns/op JsonConverterBenchmark.deserialize:p0.00false sample 16576.000 ns/op JsonConverterBenchmark.deserialize:p0.50false sample 18048.000 ns/op JsonConverterBenchmark.deserialize:p0.90false sample 21152.000 ns/op JsonConverterBenchmark.deserialize:p0.95false sample 24736.000 ns/op JsonConverterBenchmark.deserialize:p0.99false sample 37376.000 ns/op JsonConverterBenchmark.deserialize:p0.999 false sample 49472.000 ns/op JsonConverterBenchmark.deserialize:p0. false sample 450703.258 ns/op JsonConverterBenchmark.deserialize:p1.00false sample 1052672.000 ns/op JsonConverterBenchmark.serialize true sample 1489235 8396.143 ± 12.427 ns/op JsonConverterBenchmark.serialize:p0.00 true sample 7296.000 ns/op JsonConverterBenchmark.serialize:p0.50 true sample 8072.000 ns/op JsonConverterBenchmark.serialize:p0.90 true sample 8640.000 ns/op JsonConverterBenchmark.serialize:p0.95 true sample 9552.000 ns/op JsonConverterBenchmark.serialize:p0.99 true sample 15456.000 ns/op JsonConverterBenchmark.serialize:p0.999 true sample 23520.000 ns/op JsonConverterBenchmark.serialize:p0. true sample 38724.890 ns/op JsonConverterBenchmark.serialize:p1.00 true sample 1159168.000 ns/op JsonConverterBenchmark.serializefalse sample 1411941 8850.354 ± 12.808 ns/op JsonConverterBenchmark.serialize:p0.00 false sample 7552.000 ns/op JsonConverterBenchmark.serialize:p0.50 false sample 8336.000 ns/op JsonConverterBenchmark.serialize:p0.90 false sample 9456.000 ns/op JsonConverterBenchmark.serialize:p0.95 false sample 11744.000 ns/op JsonConverterBenchmark.serialize:p0.99 false sample 18464.000 ns/op JsonConverterBenchmark.serialize:p0.999 false sample 25313.856 ns/op JsonConverterBenchmark.serialize:p0.false sample 56192.000 ns/op JsonConverterBenchmark.serialize:p1.00 false sample 952320.000 ns/op ``` -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mimaison commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1862933674 Thanks @mfvitale for the PR. This looks like a significant performance improvement. We can't upgrade to Blackbird just yet as Kafka 3.X still supports Java 8. Could you write a small jmh benchmark to demonstrate the performance difference? For example, see https://github.com/apache/kafka/pull/13776 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15996: Improve JsonConverter performance [kafka]
mfvitale commented on PR #14992: URL: https://github.com/apache/kafka/pull/14992#issuecomment-1855495758 @C0urante @mimaison can you please gently have a look? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
