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-20098] Don't add flink-connector-files to flink-dist, make dependencies explicit #14122
Conversation
…endencies explicit We currently add both flink-connector-files and flink-connector-base to flink-dist. This implies, that users should use the dependency like this: <dependency> <groupId>org.apache.flink</groupId> <artifactId>flink-connector-files</artifactId> <version>${project.version}</version> <scope>provided</scope> </dependency> which differs from other connectors where users don't need to specify <scope>provided</scope>. Also, flink-connector-files had flink-connector-base as a provided dependency, which means that examples that use this dependency would not run out-of-box in IntelliJ because transitive provided dependencies will not be considered. This removes the dependencies from flink-dist which lets users use the File Connector like any other connector. I believe the initial motivation for "providing" the File Connector in flink-dist was to allow us to use the File Connector under the hood in methods such as StreamExecutionEnvironment.readFile(...). We could decide to deprecate and remove those methods or re-add the File Connector as an explicit (non-provided) dependency again in the future.
Second time might work. 😅 |
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 48788be (Wed Nov 18 13:19:34 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
48788be
to
2e98342
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, modulo one comment
<!-- <!– The SQL Client test uses a Table FileSource, which is not included in the normal table--> | ||
<!-- dependencies –>--> | ||
<!-- <dependency>--> | ||
<!-- <groupId>org.apache.flink</groupId>--> | ||
<!-- <artifactId>flink-connector-files</artifactId>--> | ||
<!-- <version>${project.version}</version>--> | ||
<!-- </dependency>--> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this? If there is a reason to leave it I can't see add a comment why do we have that commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I put this in because I thought I might have to uncomment it but I was 90% sure that it was not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why it's in a separate commit. I should have mentioned that before ... 🙈
Merged! |
[FLINK-20098] Don't add flink-connector-files to flink-dist, make dependencies explicit
We currently add both flink-connector-files and flink-connector-base to flink-dist.
This implies, that users should use the dependency like this:
which differs from other connectors where users don't need to specify provided.
Also,
flink-connector-files
hadflink-connector-base
as a provided dependency, which means that examples that use this dependency would not run out-of-box in IntelliJ because transitive provided dependencies will not be considered.This removes the dependencies from
flink-dist
which lets users use the File Connector like any other connector.I believe the initial motivation for "providing" the File Connector in
flink-dist
was to allow us to use the File Connector under the hood in methods such asStreamExecutionEnvironment.readFile(...)
. We could decide to deprecate and remove those methods or re-add the File Connector as an explicit (non-provided) dependency again in the future.Brief change log
flink-dist
provided
dependencies to instead be incompile
scopeflink-dist
. This increases the size of the Table uber jars by a couple of kilo bytes but it should be negligible.Verifying this change
Covered by existing tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
may: noDocumentation
Not applicable