Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MessageCrypto interface should not expose Netty ByteBuf class in the API #10616

Merged
merged 3 commits into from
May 19, 2021

Conversation

merlimat
Copy link
Contributor

Motivation

MessageCrypto is included in pulsar-client-api and it's exposing Netty ByteBuf in its API.

The problem is that we are shading Netty and the classes are renamed. We must not have external libraries symbols as part of our API.

The result is that Netty symbols are exposed as org.apache.pulsar.shade.io.netty... and therefore are not going to be interoperable with other libraries.

Because of this, pulsar-client-api was also included into the shaded Jars and that brings in other issues (like missing the sources/javadocs of the PulsarClient APIs).

Modifications

  • Removed Netty dependency on pulsar-client-api
  • Made breaking change in the MessageCrypto to use java.nio.ByteBuffer in API

@merlimat merlimat added type/bug The PR fixed a bug or issue reported a bug area/build labels May 17, 2021
@merlimat merlimat added this to the 2.8.0 milestone May 17, 2021
@merlimat merlimat self-assigned this May 17, 2021
@codelipenghui
Copy link
Contributor

I have added the release/note-required label for this change, this means we need to highlight in the release note since this introduced a break change.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM

but the build failed
the build failed:

Error:  COMPILATION ERROR : 
Error:  /home/runner/work/pulsar/pulsar/pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:[3090,42] method decrypt in interface org.apache.pulsar.client.api.MessageCrypto<MetadataT,BuilderT> cannot be applied to given types;
  required: java.util.function.Supplier,java.nio.ByteBuffer,java.nio.ByteBuffer,org.apache.pulsar.client.api.CryptoKeyReader
  found: ()->messageMetadata,io.netty.buffer.ByteBuf,org.apache.pulsar.client.api.CryptoKeyReader
  reason: actual and formal argument lists differ in length
Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:testCompile (default-testCompile) on project pulsar-broker: Compilation failure
Error:  /home/runner/work/pulsar/pulsar/pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:[3090,42] method decrypt in interface org.apache.pulsar.client.api.MessageCrypto<MetadataT,BuilderT> cannot be applied to given types;
Error:    required: java.util.function.Supplier,java.nio.ByteBuffer,java.nio.ByteBuffer,org.apache.pulsar.client.api.CryptoKeyReader
Error:    found: ()->messageMetadata,io.netty.buffer.ByteBuf,org.apache.pulsar.client.api.CryptoKeyReader
Error:    reason: actual and formal argument lists differ in length
Error:  -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.

@jiazhai
Copy link
Member

jiazhai commented May 18, 2021

Overall LGTM

but the build failed
the build failed:

Error:  COMPILATION ERROR : 
Error:  /home/runner/work/pulsar/pulsar/pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:[3090,42] method decrypt in interface org.apache.pulsar.client.api.MessageCrypto<MetadataT,BuilderT> cannot be applied to given types;
  required: java.util.function.Supplier,java.nio.ByteBuffer,java.nio.ByteBuffer,org.apache.pulsar.client.api.CryptoKeyReader
  found: ()->messageMetadata,io.netty.buffer.ByteBuf,org.apache.pulsar.client.api.CryptoKeyReader
  reason: actual and formal argument lists differ in length
Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:testCompile (default-testCompile) on project pulsar-broker: Compilation failure
Error:  /home/runner/work/pulsar/pulsar/pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java:[3090,42] method decrypt in interface org.apache.pulsar.client.api.MessageCrypto<MetadataT,BuilderT> cannot be applied to given types;
Error:    required: java.util.function.Supplier,java.nio.ByteBuffer,java.nio.ByteBuffer,org.apache.pulsar.client.api.CryptoKeyReader
Error:    found: ()->messageMetadata,io.netty.buffer.ByteBuf,org.apache.pulsar.client.api.CryptoKeyReader
Error:    reason: actual and formal argument lists differ in length
Error:  -> [Help 1]
Error:  
Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.

looks like we need care some implementation in the tests. but this change will make the management easier. +1.

@eolivelli eolivelli merged commit 840387b into apache:master May 19, 2021
@merlimat merlimat deleted the fix-message-crypto-api branch May 19, 2021 16:03
@merlimat merlimat restored the fix-message-crypto-api branch May 19, 2021 16:03
@merlimat merlimat deleted the fix-message-crypto-api branch May 19, 2021 16:04
yangl pushed a commit to yangl/pulsar that referenced this pull request Jun 23, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants