-
Notifications
You must be signed in to change notification settings - Fork 252
feat: sbt-ci-release #598
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
feat: sbt-ci-release #598
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #598 +/- ##
==========================================
- Coverage 91.43% 89.85% -1.58%
==========================================
Files 18 21 +3
Lines 829 1065 +236
Branches 52 119 +67
==========================================
+ Hits 758 957 +199
- Misses 71 108 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
To check the result:
|
With a single command: ./build/sbt "root/publishLocal; connect/publishLocal; databricksConnect/publishLocal" |
Is there an issue for why we need databricks-specific logic? should we link that explanation here in the PR? |
mrpowers-io/tsumugi-spark#55 (comment) For unknown for me reason, databricks engineers are using an own shading rule that is different from an OSS one. |
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 really dont like the idea of having a databricks specific distribution. However I don't have a better option here. I asked around a little bit if there is something else we can do that is vendor agnostic.
I will approve once the databricks typo is fixed
@WeichenXu123 Hello! Do you know, can we use the word "databricks" in the name of published artifacts ( cc: @rjurney |
LGTM! |
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.
lgtm
I'm waiting for the approve from @dmatrix about can we use the word "databricks" in the name of released artifacts, code and documentation. |
I suggest to avoid using "databricks" in the name, but using name like but I think supporting Spark connect is not sufficient to make it work on Databricks shared cluster, we also need to whitelist a bunch of JVM methods in the lib for running on Databricks shared cluster (and needs our security team's approval for doing that) |
Does the API change from protobuf.Any to Array[Byte] in Spark 4 fix the shading/binary compatibility issue, at least in Spark 4+? Might be a good compromise if it does to prevent a dedicated artifact for a closed source fork |
I did not try but for me it looks like this change in Spark 4.0.x should resolve the mentioned problem. |
@james-willis @Kimahriman @rjurney @dmatrix Hello! After some discussions with engineers from Databricks and based on your comments I did the following updates:
Please, take a look on the PR when you have time. cc: @WeichenXu123 |
I looked into what sedona is doing here and it seems that it shades in its own version of dependencies when databricks has some kind of conflict: https://github.com/apache/sedona/blob/e8c0d5af41670757c6d319d9c0d393b0154ec929/common/pom.xml#L143 |
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.
will making users compile from source for dbx impede adoption?
Running Spark Connect plugins on Databricks is not an easy task anyway. I think we should be fine. Spark 4.0 should resolve this. |
What changes were proposed in this pull request?
build.sbt
databricks-connect
subprojectWhy are the changes needed?
Part of #510