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

[FLINK-32522][connectors/kafka] Kafka connector should depend on commons-collections instead of inheriting from flink #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chucheng92
Copy link
Member

@chucheng92 chucheng92 commented Jul 3, 2023

What is the purpose of the change

As this thread https://lists.apache.org/thread/l98pc18onxrcrsb01x5kh1vppl7ymk2d discussed.
Connectors shouldn't rely on dependencies that may or may not be
available in Flink itself. But currently kafka connector use commons-collections from flink, we should depend on commons-collections and bundle it in shaded-jar. otherwise, connector will be vulnerable and even block upgrading of flink.

Brief change log

Add commons-collections in pom

Verifying this change

This change is a trivial rework / code cleanup. Verifying by current total cases.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): yes
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@chucheng92 chucheng92 changed the title [FLINK-32522] Add commons-collections in kafka-sql-connector shade jar [FLINK-32522][connectors/kafka] Kafka connector should depend on commons-collections instead of inherit from flink Jul 4, 2023
@chucheng92
Copy link
Member Author

chucheng92 commented Jul 4, 2023

@MartijnVisser @XComp Hi, guys. PTAL. thanks

@chucheng92
Copy link
Member Author

@MartijnVisser hi, Martijn. Can you help to review it?

Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

@chucheng92 Looking at the dependency tree, there's still commons-collections coming in via Flink itself. If we want to remove the dependency, we should also make sure that it's not included from there

@chucheng92
Copy link
Member Author

chucheng92 commented Oct 11, 2023

@chucheng92 Looking at the dependency tree, there's still commons-collections coming in via Flink itself. If we want to remove the dependency, we should also make sure that it's not included from there

Yes. I have updated the PR.

old commons-collections in kafka connector is inherited from flink (so we will see it in kafka). This is a circular dependency and we should break it.

What I am thinking about now is to change the kafka connector to commons-collections4. When the kafka connector is released, upgrade the kafka-connector that Python depends on to this version on the flink side. In this way, it is decoupled, and the flink side can also be upgraded to commons-collections4. So the apache/flink#21442 can be resolved. And it will remove all the old commons-collections from flink side.

@chucheng92
Copy link
Member Author

@chucheng92 Looking at the dependency tree, there's still commons-collections coming in via Flink itself. If we want to remove the dependency, we should also make sure that it's not included from there

Of course, the more important point is that pyflink should not rely on connectors, even for integration tests

…ons-collections instead of inheriting from flink
@chucheng92 chucheng92 changed the title [FLINK-32522][connectors/kafka] Kafka connector should depend on commons-collections instead of inherit from flink [FLINK-32522][connectors/kafka] Kafka connector should depend on commons-collections instead of inheriting from flink Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants