You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/13 15:54:59 UTC
[GitHub] [kafka] ijuma opened a new pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
ijuma opened a new pull request #10123:
URL: https://github.com/apache/kafka/pull/10123
We don't really need it and it causes problems in older Android versions
and GraalVM usage.
Move the logic to separate classes that are only invoked when the
relevant compression library is actually used. Place such classes
in their own package and enforce via checkstyle that only these
classes refer to compression library packages.
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#discussion_r575697037
##########
File path: checkstyle/import-control.xml
##########
@@ -144,15 +155,14 @@
</subpackage>
<subpackage name="record">
- <allow pkg="net.jpountz" />
+ <allow pkg="org.apache.kafka.common.compression" />
Review comment:
Could we move `CompressionType` to `org.apache.kafka.common.compression` and then make both `ZstdFactory` and SnappyFactory be package-private?
##########
File path: checkstyle/import-control.xml
##########
@@ -69,6 +69,17 @@
<allow pkg="org.apache.kafka.common.metrics" />
</subpackage>
+ <!-- Third-party compression libraries should only be references from this package -->
+ <subpackage name="compression">
+ <allow pkg="com.github.luben.zstd" />
+ <allow pkg="net.jpountz.lz4" />
+ <allow pkg="net.jpountz.xxhash" />
+ <allow pkg="org.apache.kafka.common.compression" />
+ <allow pkg="org.xerial.snappy" />
+ <!-- For testing -->
Review comment:
This comment is a bit weird since `ZstdFactory` does reference the `org.apache.kafka.common.record.BufferSupplier`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#issuecomment-778799315
JDK 15 build passed, JDK 8 had unrelated flaky failures and JDK 11 died abruptly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma merged pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #10123:
URL: https://github.com/apache/kafka/pull/10123
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#issuecomment-778797029
@chia7712 with these changes, older Android clients should be able to start (before they would fail when the method handles code was reached even if no compression was actually used). If compressed data is used, then it depends on whether the native libraries are available. If not, then it would still fail.
Today, the solution would be to either:
1. Include the relevant native compression libraries
2. Limit topic data to the compression algorithms supported on the relevant platform
Both seem doable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#discussion_r575703615
##########
File path: checkstyle/import-control.xml
##########
@@ -69,6 +69,17 @@
<allow pkg="org.apache.kafka.common.metrics" />
</subpackage>
+ <!-- Third-party compression libraries should only be references from this package -->
+ <subpackage name="compression">
+ <allow pkg="com.github.luben.zstd" />
+ <allow pkg="net.jpountz.lz4" />
+ <allow pkg="net.jpountz.xxhash" />
+ <allow pkg="org.apache.kafka.common.compression" />
+ <allow pkg="org.xerial.snappy" />
+ <!-- For testing -->
Review comment:
That's a good point. It probably means we need to move BufferSupplier to another package to avoid circular dependencies.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#discussion_r575764462
##########
File path: checkstyle/import-control.xml
##########
@@ -144,15 +155,14 @@
</subpackage>
<subpackage name="record">
- <allow pkg="net.jpountz" />
+ <allow pkg="org.apache.kafka.common.compression" />
Review comment:
make sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#discussion_r575703770
##########
File path: checkstyle/import-control.xml
##########
@@ -144,15 +155,14 @@
</subpackage>
<subpackage name="record">
- <allow pkg="net.jpountz" />
+ <allow pkg="org.apache.kafka.common.compression" />
Review comment:
Possibly. Let me see what other impact it has.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#issuecomment-778799711
Merged to trunk and 2.8.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#issuecomment-780326554
I'm not sure it's worth it since the cost of compressing on the broker during fetches is significantly higher than compressing during produce (the data is already on the heap for the latter and there are usually multiple fetches per produce). That is not to say that it's never useful, just that the ROI seems a bit low.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#issuecomment-778637526
@chia7712 Thoughts on this change?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#discussion_r575719316
##########
File path: checkstyle/import-control.xml
##########
@@ -69,6 +69,17 @@
<allow pkg="org.apache.kafka.common.metrics" />
</subpackage>
+ <!-- Third-party compression libraries should only be references from this package -->
+ <subpackage name="compression">
+ <allow pkg="com.github.luben.zstd" />
+ <allow pkg="net.jpountz.lz4" />
+ <allow pkg="net.jpountz.xxhash" />
+ <allow pkg="org.apache.kafka.common.compression" />
+ <allow pkg="org.xerial.snappy" />
+ <!-- For testing -->
Review comment:
Addressed this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#issuecomment-778666677
@chia7712 Both solutions should behave the same at runtime. At compile time, the previous solution did not require snappy libraries in the classpath (but it did require lz4 and zstd). So, overall, I think there is no difference that matters.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#discussion_r575719312
##########
File path: checkstyle/import-control.xml
##########
@@ -144,15 +155,14 @@
</subpackage>
<subpackage name="record">
- <allow pkg="net.jpountz" />
+ <allow pkg="org.apache.kafka.common.compression" />
Review comment:
This affects a lot of files and I'd rather do that in a separate PR as I may backport this one to stable branches.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #10123: KAFKA-12327: Remove MethodHandle usage in CompressionType
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10123:
URL: https://github.com/apache/kafka/pull/10123#issuecomment-780303685
> Today, the solution would be to either:
Include the relevant native compression libraries
Limit topic data to the compression algorithms supported on the relevant platform
Both seem doable.
@ijuma Thanks for the sharing. IIRC, kafka producer which does not support the compression can send uncompressed data to server. The data get compressed on server-side. It is a useful feature since the compression does not obstruct us from producing data on env which can't load native compression libraries. In contrast with producer, kafka consumer can't get data from compressed topic if it can't load native compression libraries. WDYT? Does it pay to support such scenario?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org