Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Feb 14, 2024

What changes were proposed in this pull request?

This PR aims to remove the shaded protobuf-java 2.6.1 dependency from kinesis-asl-assembly.

Why are the changes needed?

Technically, this is a revert of SPARK-14421.

Does this PR introduce any user-facing change?

No, but the user can inject the required protobuf-jar from application layers.

How was this patch tested?

Pass the CIs and manual review.

BEFORE

$ build/mvn dependency:tree -pl connector/kinesis-asl-assembly -Pkinesis-asl | grep protobuf-java
...
[INFO] +- com.google.protobuf:protobuf-java:jar:2.6.1:provided

AFTER

$ build/mvn dependency:tree -pl connector/kinesis-asl-assembly -Pkinesis-asl | grep protobuf-java
...
[INFO] |  |  +- com.google.protobuf:protobuf-java:jar:3.25.1:provided

Was this patch authored or co-authored using generative AI tooling?

No.

@dongjoon-hyun dongjoon-hyun marked this pull request as draft February 14, 2024 10:11
@github-actions github-actions bot added the BUILD label Feb 14, 2024
@dongjoon-hyun dongjoon-hyun marked this pull request as ready for review February 14, 2024 10:58
@bjornjorgensen
Copy link
Contributor

Do we run test on Kinesis?

@dongjoon-hyun
Copy link
Member Author

Yes

@dongjoon-hyun
Copy link
Member Author

All tests passed.

@dongjoon-hyun
Copy link
Member Author

Could you review this, please, @viirya ?

@viirya
Copy link
Member

viirya commented Feb 14, 2024

Looking into this.

Comment on lines -62 to -71
<dependency>
<groupId>com.google.protobuf</groupId>
<artifactId>protobuf-java</artifactId>
<version>2.6.1</version>
<!--
We are being explicit about version here and overriding the
spark default of 2.5.0 because KCL appears to have introduced
a dependency on protobuf 2.6.1 somewhere between KCL 1.4.0 and 1.6.1.
-->
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

Before SPARK-14421, it is a provided dependency. Don't we need to keep it?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Feb 14, 2024

Choose a reason for hiding this comment

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

Yes, it's provided dependency still as I described in the PR description.
Screenshot 2024-02-14 at 09 58 01

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. It was overriding the protobuf version.

@dongjoon-hyun
Copy link
Member Author

Thank you so much, @viirya !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants