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

[CELEBORN-836][BUILD] Initial support sbt #1757

Closed
wants to merge 44 commits into from

Conversation

cfmcgrady
Copy link
Contributor

@cfmcgrady cfmcgrady commented Jul 24, 2023

What changes were proposed in this pull request?

This PR introduces the SBT build system implementation that operates independently from the current Maven build system. Different from #1627, the current implementation does not depend on pom.xml

The implementation enables packaging and testing functionalities for server-related modules and Spark-related modules using SBT.

For Flink-related build/test, sbt build documentation, continuous integration, and plugins, they will be submitted in separate PRs

Why are the changes needed?

improve project build speed

packing the project.

$ ./build/sbt
sbt:celeborn> clean
[success] Total time: 1 s, completed 2023-7-25 16:36:12
sbt:celeborn> package
[success] Total time: 28 s, completed 2023-7-25 16:36:46

packing and shading the spark 3.3 client

$ ./build/sbt -Pspark-3.3
sbt:celeborn> clean
[success] Total time: 1 s, completed 2023-7-25 16:39:11
sbt:celeborn> project celeborn-client-spark-3-shaded
sbt:celeborn-client-spark-3-shaded> assembly
[success] Total time: 37 s, completed 2023-7-25 16:40:03

packing and shading the spark 2.4 client

$ ./build/sbt -Pspark-2.4
sbt:celeborn> clean
[success] Total time: 1 s, completed 2023-7-25 16:41:06
sbt:celeborn> project celeborn-client-spark-2-shaded
sbt:celeborn-client-spark-2-shaded> assembly
[success] Total time: 36 s, completed 2023-7-25 16:41:53

running server-related tests

$ ./build/sbt clean test
[success] Total time: 350 s (05:50), completed 2023-7-25 16:48:58

Does this PR introduce any user-facing change?

yes

How was this patch tested?

tested locally

@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #1757 (fab0da0) into main (6427ed3) will increase coverage by 0.08%.
Report is 9 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1757      +/-   ##
==========================================
+ Coverage   46.72%   46.79%   +0.08%     
==========================================
  Files         161      162       +1     
  Lines       10021    10046      +25     
  Branches      925      927       +2     
==========================================
+ Hits         4681     4700      +19     
- Misses       5021     5026       +5     
- Partials      319      320       +1     

see 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -0,0 +1,7 @@
[repositories]
Copy link
Member

Choose a reason for hiding this comment

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

does the file support comments? we can add comments to guide user how to use this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pan3793
Copy link
Member

pan3793 commented Jul 26, 2023

@waitinfuture @RexXiong @FMX @AngersZhuuuu would you please take a look?

note: if you encounter network issue, run the following command to apply mirror

cp build/sbt-config/repositories-cn.template build/sbt-config/repositories-local


usage() {
cat <<EOM
Usage: $script_name [options]
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is not the same as sbt official options, we may want to migrate it to the official one in the future.


[repositories]
local
private: ${DEFAULT_ARTIFACT_REPOSITORY-file:///dev/null}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user has configured it, include the DEFAULT_ARTIFACT_REPOSITORY in the list of repositories. cc @pan3793

the variable substitution docs: https://www.scala-sbt.org/1.x/docs/Launcher-Configuration.html#Variable+Substitution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the variable substitution required system property instead of the environment variable, updated in 48a1aa9

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for fixing it.

@FMX
Copy link
Contributor

FMX commented Jul 27, 2023

I tried assembly and I got errors. Is there anything that I missed?

[info] 2 file(s) merged using strategy 'Rename' (Run the task at debug level to see the details)
[info] 1 file(s) merged using strategy 'Discard' (Run the task at debug level to see the details)
[info] Built: /Users/minxfeng/wcode/apache/incubator-celeborn/target/scala-2.12/incubator-celeborn-assembly-0.4.0-SNAPSHOT.jar
[info] Jar hash: 6bec02bf548084ceb7c97f30458481d512ab2363
[error] 9 error(s) were encountered during the merge:
[error] 9 error(s) were encountered during the merge:
[error] 9 error(s) were encountered during the merge:
[error] 11 error(s) were encountered during the merge:
[error] 11 error(s) were encountered during the merge:
[error] stack trace is suppressed; run last common / assembly for the full output
[error] stack trace is suppressed; run last client / assembly for the full output
[error] stack trace is suppressed; run last worker / assembly for the full output
[error] stack trace is suppressed; run last master / assembly for the full output
[error] stack trace is suppressed; run last service / assembly for the full output
[error] (common / assembly)
[error] Deduplicate found different file contents in the following:
[error] Jar name = commons-logging-1.1.3.jar, jar org = commons-logging, entry target = org/apache/commons/logging/impl/SimpleLog$1.class
[error] Jar name = jcl-over-slf4j-1.7.36.jar, jar org = org.slf4j, entry target = org/apache/commons/logging/impl/SimpleLog$1.class
[error] Deduplicate found different file contents in the following:
[error] Jar name = commons-logging-1.1.3.jar, jar org = commons-logging, entry target = org/apache/commons/logging/Log.class
[error] Jar name = jcl-over-slf4j-1.7.36.jar, jar org = org.slf4j, entry target = org/apache/commons/logging/Log.class

@cfmcgrady
Copy link
Contributor Author

@FMX
the assembly command only works on shaded-related modules now, let's switch the project first.

$ ./build/sbt -Pspark-3.4
sbt:celeborn> projects
[info] In file:/home/chenfu/workspace/celeborn/
[info]   * celeborn
[info]     celeborn-client-spark-3
[info]     celeborn-client-spark-3-shaded
[info]     client
[info]     common
[info]     master
[info]     service
[info]     spark-common
[info]     spark-it
[info]     worker
sbt:celeborn> project celeborn-client-spark-3-shaded
sbt:celeborn-client-spark-3-shaded> assembly

or

$ ./build/sbt -Pspark-3.4 celeborn-client-spark-3-shaded/assembly

@pan3793
Copy link
Member

pan3793 commented Jul 28, 2023

Thanks, merging to main

@pan3793 pan3793 closed this in 5f0295e Jul 28, 2023
@cfmcgrady cfmcgrady deleted the pure-sbt branch July 28, 2023 05:58
@waitinfuture
Copy link
Contributor

Thanks @cfmcgrady for this PR! I tested and it really speeded up build time. Just wonder do you have a plan to add a doc describing building Celeborn?

@cfmcgrady
Copy link
Contributor Author

@waitinfuture
Copy link
Contributor

That's great! Thanks @cfmcgrady

pan3793 pushed a commit that referenced this pull request Aug 1, 2023
### What changes were proposed in this pull request?

This PR adds packaging and testing support for Flink-related modules using SBT based on #1757

### Why are the changes needed?

improve project build speed

running flink-it tests with -Pflink-1.14

```shell
sbt:celeborn> project flink-it
sbt:flink-it> clean
sbt:flink-it> test
[success] Total time: 136 s (02:16), completed 2023-7-27 11:55:10
```

running flink-it tests with -Pflink-1.17

```shell
$ ./build/sbt -Pflink-1.17
sbt:celeborn> project flink-it
sbt:flink-it> clean
sbt:flink-it> test
[success] Total time: 168 s (02:48), completed 2023-7-27 11:28:35
```

packing and shading the flink 1.14 client

```shell
$ ./build/sbt -Pflink-1.14
sbt:celeborn> clean
sbt:celeborn> project celeborn-client-flink-1_14-shaded
sbt:celeborn-client-flink-1_14-shaded> assembly
[success] Total time: 35 s, completed 2023-7-27 11:51:54
```

packing and shading the flink 1.17 client

```shell
$ ./build/sbt -Pflink-1.17
sbt:celeborn> clean
sbt:celeborn> project celeborn-client-flink-1_17-shaded
sbt:celeborn-client-flink-1_17-shaded> assembly
[success] Total time: 39 s, completed 2023-7-27 11:49:20
```

### Does this PR introduce _any_ user-facing change?

yes

### How was this patch tested?

tested locally

Closes #1764 from cfmcgrady/sbt-flink.

Authored-by: Fu Chen <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
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