-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Keep Spark version in a single place only (BeamModulePlugin) #23603
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23603 +/- ##
=======================================
Coverage 73.33% 73.33%
=======================================
Files 719 719
Lines 95792 95792
=======================================
Hits 70250 70250
Misses 24231 24231
Partials 1311 1311
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Retest this please |
Run Spark StructuredStreaming ValidatesRunner |
Run Spark ValidatesRunner |
Run Java_Debezium_IO_Direct PreCommit |
Run Portable_Python PreCommit |
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.
Thanks, LGTM. Just a couple of notes.
@@ -499,6 +499,10 @@ class BeamModulePlugin implements Plugin<Project> { | |||
def arrow_version = "5.0.0" | |||
def jmh_version = "1.34" | |||
|
|||
// Export Spark versions, so they are defined in a single place only | |||
project.ext.spark2_version = spark2_version |
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.
nit: It'd be worth to mention that spark 2 is deprecated in Beam
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.
I'll add some comments as part of #23728
// Set the version of all Spark-related dependencies here. | ||
spark_version = '2.4.8' | ||
// Spark 2 version as defined in BeamModulePlugin | ||
spark_version = spark2_version |
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.
As a side note - it's ambiguous to have two types of the naming style (underscore and camel case) in the same config file.
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.
Not going to fix all inconsistencies at once ;)
It's a bit of a trap to have the Spark version in two places.
Especially because it's most obvious to change the one in the Spark build.gradle. But that one is always overruled by the one used in the artifact library in BeamModulePlugin.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.