KAFKA-12327: Remove MethodHandle usage in CompressionType by ijuma · Pull Request #10123 · apache/kafka

ijuma · 2021-02-13T15:54:51Z

We don't really need it and it causes problems in older Android versions
and GraalVM native image usage (there are workarounds for the latter).

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.

To avoid cyclic dependencies, moved BufferSupplier to the utils
package.

Committer Checklist (excluded from commit message)

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

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.

ijuma · 2021-02-13T15:58:14Z

@chia7712 Thoughts on this change?

chia7712

Does previous solution (dynamically find constructor) still load native library?

chia7712 · 2021-02-13T18:13:00Z

+      <allow pkg="net.jpountz.xxhash" />
+      <allow pkg="org.apache.kafka.common.compression" />
+      <allow pkg="org.xerial.snappy" />
+      <!-- For testing -->


This comment is a bit weird since ZstdFactory does reference the org.apache.kafka.common.record.BufferSupplier.

That's a good point. It probably means we need to move BufferSupplier to another package to avoid circular dependencies.

Addressed this.

chia7712 · 2021-02-13T18:19:32Z


    <subpackage name="record">
-      <allow pkg="net.jpountz" />
+      <allow pkg="org.apache.kafka.common.compression" />


Could we move CompressionType to org.apache.kafka.common.compression and then make both ZstdFactory and SnappyFactory be package-private?

Possibly. Let me see what other impact it has.

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.

make sense.

ijuma · 2021-02-13T19:30:40Z

@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.

chia7712

LGTM

BTW, is it an issue that kafka client running on either older Android versions or GraalVM can't consume compressed data from brokers? Maybe broker can decompress those data on server-side for those clients which can't support compression.

ijuma · 2021-02-14T15:54:00Z

@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:

Include the relevant native compression libraries
Limit topic data to the compression algorithms supported on the relevant platform

Both seem doable.

ijuma · 2021-02-14T16:10:49Z

JDK 15 build passed, JDK 8 had unrelated flaky failures and JDK 11 died abruptly.

ijuma · 2021-02-14T16:13:45Z

Merged to trunk and 2.8.

We don't really need it and it causes problems in older Android versions and GraalVM native image usage (there are workarounds for the latter). 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. To avoid cyclic dependencies, moved `BufferSupplier` to the `utils` package. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>

…e-allocations-lz4 * apache-github/trunk: (118 commits) KAFKA-12327: Remove MethodHandle usage in CompressionType (apache#10123) KAFKA-12297: Make MockProducer return RecordMetadata with values as per contract MINOR: Update zstd and use classes with no finalizers (apache#10120) KAFKA-12326: Corrected regresion in MirrorMaker 2 executable introduced with KAFKA-10021 (apache#10122) KAFKA-12321 the comparison function for uuid type should be 'equals' rather than '==' (apache#10098) MINOR: Add FetchSnapshot API doc in KafkaRaftClient (apache#10097) MINOR: KIP-631 KafkaConfig fixes and improvements (apache#10114) KAFKA-12272: Fix commit-interval metrics (apache#10102) MINOR: Improve confusing admin client shutdown logging (apache#10107) MINOR: Add BrokerMetadataListener (apache#10111) MINOR: Support Raft-based metadata quorums in system tests (apache#10093) MINOR: add the MetaLogListener, LocalLogManager, and Controller interface. (apache#10106) MINOR: Introduce the KIP-500 Broker lifecycle manager (apache#10095) MINOR: Remove always-passing validation in TestRecordTest#testProducerRecord (apache#9930) KAFKA-5235: GetOffsetShell: Support for multiple topics and consumer configuration override (KIP-635) (apache#9430) MINOR: Prevent creating partition.metadata until ID can be written (apache#10041) MINOR: Add RaftReplicaManager (apache#10069) MINOR: Add ClientQuotaMetadataManager for processing QuotaRecord (apache#10101) MINOR: Rename DecommissionBrokers to UnregisterBrokers (apache#10084) MINOR: KafkaBroker.brokerState should be volatile instead of AtomicReference (apache#10080) ... clients/src/main/java/org/apache/kafka/common/record/CompressionType.java core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala

chia7712 · 2021-02-17T05:12:42Z

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?

ijuma · 2021-02-17T06:09:13Z

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.

Kafka 2.8.0 removes the `MethodHandle` usage in `CompressionType` (see apache/kafka#10123) removing `SnappyConstructors` as well. As a result the substitution is no longer valid. The update to Kafka 2.8.0 was done in quarkusio@f8ce993 Fixes quarkusio#16721

Kafka 2.8.0 removes the `MethodHandle` usage in `CompressionType` (see apache/kafka#10123) removing `SnappyConstructors` as well. As a result the substitution is no longer valid. The update to Kafka 2.8.0 was done in quarkusio/quarkus@f8ce993 Fixes #16721

chia7712 reviewed Feb 13, 2021

View reviewed changes

ijuma added 4 commits February 13, 2021 13:54

Rename package compression to compress

8efdfb6

Move BufferSupplier to utils package to avoid cyclic dependencies

5916037

Revert unnecessary changes

010f505

Minor formatting fix

c5d7bc2

chia7712 approved these changes Feb 14, 2021

View reviewed changes

ijuma merged commit 744d05b into apache:trunk Feb 14, 2021

ijuma deleted the remove-method-handle-compression-type branch February 14, 2021 16:12

zakkak mentioned this pull request Apr 26, 2021

Remove obsolete substitution for kafka snappy quarkusio/quarkus#16798

Merged

Conversation

ijuma commented Feb 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented Feb 13, 2021

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

chia7712 Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

ijuma Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

ijuma Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

chia7712 Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

ijuma Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

ijuma Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

chia7712 Feb 14, 2021

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 13, 2021

Uh oh!

chia7712 left a comment

Choose a reason for hiding this comment

Uh oh!

ijuma commented Feb 14, 2021

Uh oh!

ijuma commented Feb 14, 2021

Uh oh!

ijuma commented Feb 14, 2021

Uh oh!

chia7712 commented Feb 17, 2021

Uh oh!

ijuma commented Feb 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ijuma commented Feb 13, 2021 •

edited

Loading