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

[BUILD] Reduce unnecessary maven profile #1257

Closed
wants to merge 15 commits into from
Closed

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Oct 19, 2021

Why are the changes needed?

The changes based on #1226 (comment)

In this PR, I'm going to remove profiles kyuubi-extension-spark-3-1 and kyuubi-extension-spark-3-2, and keep spark-3.0, spark-3.1, spark-3.2.

After changes, when spark-3.1 is active, set spark.version to 3.1.2 and enable kyuubi-extension-spark-common module and kyuubi-extension-spark-3-1 module, spark-3.2 does the same thing

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

@pan3793 pan3793 self-assigned this Oct 19, 2021
@pan3793 pan3793 added the infra label Oct 19, 2021
@pan3793 pan3793 added this to the v1.4.0 milestone Oct 19, 2021
@pan3793 pan3793 mentioned this pull request Oct 19, 2021
14 tasks
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2021

Codecov Report

Merging #1257 (1d871b6) into master (d0d5fb6) will increase coverage by 5.11%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1257      +/-   ##
============================================
+ Coverage     73.10%   78.22%   +5.11%     
- Complexity       41      110      +69     
============================================
  Files           175      177       +2     
  Lines          6846     6940      +94     
  Branches        835      852      +17     
============================================
+ Hits           5005     5429     +424     
+ Misses         1441     1043     -398     
- Partials        400      468      +68     
Impacted Files Coverage Δ
...che/kyuubi/ha/client/ZooKeeperClientProvider.scala 70.17% <0.00%> (-11.88%) ⬇️
...ache/kyuubi/engine/spark/SparkProcessBuilder.scala 86.86% <0.00%> (-2.91%) ⬇️
.../scala/org/apache/kyuubi/server/KyuubiServer.scala 53.48% <0.00%> (-2.33%) ⬇️
...n/scala/org/apache/kyuubi/engine/ProcBuilder.scala 89.89% <0.00%> (-1.02%) ⬇️
...apache/kyuubi/ha/client/ZooKeeperACLProvider.scala 85.00% <0.00%> (-0.72%) ⬇️
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 95.22% <0.00%> (-0.02%) ⬇️
...in/scala/org/apache/kyuubi/server/api/v1/dto.scala 0.00% <0.00%> (ø)
...apache/kyuubi/server/api/v1/SessionsResource.scala 0.00% <0.00%> (ø)
...g/apache/kyuubi/engine/spark/udf/KDFRegistry.scala 100.00% <0.00%> (ø)
...g/apache/kyuubi/ha/client/ZooKeeperAuthTypes.scala 100.00% <0.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0d5fb6...1d871b6. Read the comment docs.

@yaooqinn
Copy link
Member

Copy the comment or make a summary here in PR desc?

Cheng Pan and others added 2 commits October 20, 2021 14:29
./build/mvn clean deploy -DskipTests -Pkyuubi-extension-spark-3-1,spark-provided -s ./build/release/asf-settings.xml
./build/mvn clean deploy -s ./build/release/asf-settings.xml -DskipTests -Pspark-provided
./build/mvn clean deploy -s ./build/release/asf-settings.xml -DskipTests -Pspark-3.1 -s ./build/release/asf-settings.xml -pl dev/kyuubi-extension-spark-3-1 -am
./build/mvn clean deploy -s ./build/release/asf-settings.xml -DskipTests -Pspark-3.2 -s ./build/release/asf-settings.xml -pl dev/kyuubi-extension-spark-3-2 -am
Copy link
Member Author

Choose a reason for hiding this comment

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

L39: build against the default spark version, publish no kyuubi spark extension modules
L40: only build kyuubi-extension-spark-3-1 and dependent modules against spark 3.1, publish kyuubi-extension-spark-3-1
L41: only build kyuubi-extension-spark-3-2 and dependent modules against spark 3.2, publish kyuubi-extension-spark-3-2

Copy link
Contributor

Choose a reason for hiding this comment

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

is it allowed if we release the binary that contains Spark binary ? before we only release Kyuubi without Spark.

-pl dev/kyuubi-extension-spark-3-1 -am
${KYUUBI_DIR}/build/mvn clean deploy -DskipTests -Papache-release,spark-3.2 \
-s "${KYUUBI_DIR}/build/release/asf-settings.xml" \
-pl dev/kyuubi-extension-spark-3-2 -am
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as GA

@pan3793 pan3793 requested a review from yaooqinn October 20, 2021 12:09
@pan3793
Copy link
Member Author

pan3793 commented Oct 22, 2021

cc @yaooqinn @ulysses-you all comments have been addressed, please review again.

@ulysses-you
Copy link
Contributor

thanks, merging to master

@pan3793 pan3793 deleted the pom branch October 22, 2021 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants