Skip to content

Conversation

@EnricoMi
Copy link
Contributor

@EnricoMi EnricoMi commented Feb 1, 2024

What changes were proposed in this pull request?

The dashboard module should be moved into a profile and and not build by default.

It can be activated via -Pdashbboard.

Why are the changes needed?

Building and especially incrementally compiling during development always builds dashboard completely. All other modules finish in seconds, but dashboard takes minutes. This increases the iteration cycle from seconds to minutes.

Compiling Uniffle with dashboard:

./mvnw compile -Pdashboard
…
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache Uniffle 0.9.0-SNAPSHOT:
[INFO] 
[INFO] Apache Uniffle ..................................... SUCCESS [  0.804 s]
[INFO] Apache Uniffle Protocols ........................... SUCCESS [  4.486 s]
[INFO] Apache Uniffle Common .............................. SUCCESS [  1.778 s]
[INFO] Apache Uniffle Internal Client ..................... SUCCESS [  0.244 s]
[INFO] Apache Uniffle Coordinator ......................... SUCCESS [  0.492 s]
[INFO] Apache Uniffle Storage ............................. SUCCESS [  1.433 s]
[INFO] Apache Uniffle Server .............................. SUCCESS [  1.161 s]
[INFO] Apache Uniffle Client .............................. SUCCESS [  0.381 s]
[INFO] Apache Uniffle CLI ................................. SUCCESS [  0.107 s]
[INFO] Apache Uniffle Integration Test (Common) ........... SUCCESS [  1.066 s]
[INFO] Apache Uniffle Dashboard ........................... SUCCESS [02:22 min]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  02:35 min

Compiling Uniffle without dashboard:

./mvnw compile
…
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Apache Uniffle 0.9.0-SNAPSHOT:
[INFO] 
[INFO] Apache Uniffle ..................................... SUCCESS [  0.820 s]
[INFO] Apache Uniffle Protocols ........................... SUCCESS [  4.560 s]
[INFO] Apache Uniffle Common .............................. SUCCESS [  1.825 s]
[INFO] Apache Uniffle Internal Client ..................... SUCCESS [  0.213 s]
[INFO] Apache Uniffle Coordinator ......................... SUCCESS [  0.496 s]
[INFO] Apache Uniffle Storage ............................. SUCCESS [  1.403 s]
[INFO] Apache Uniffle Server .............................. SUCCESS [  1.100 s]
[INFO] Apache Uniffle Client .............................. SUCCESS [  0.431 s]
[INFO] Apache Uniffle CLI ................................. SUCCESS [  0.106 s]
[INFO] Apache Uniffle Integration Test (Common) ........... SUCCESS [  1.052 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  12.538 s
…


### Does this PR introduce _any_ user-facing change?
Only affects build time.

### How was this patch tested?
Manual testing.

Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

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

LGTM. Nice improvement! @EnricoMi

@zuston zuston merged commit e3b4618 into apache:master Feb 2, 2024
@rickyma
Copy link
Contributor

rickyma commented Feb 13, 2024

Currently, when running the build_distribution.sh script, it always compiles the dashboard, and there is no option to choose not to compile it. I think the build_distribution.sh script should support an option to not compile the dashboard.

WDYT? @EnricoMi

@EnricoMi
Copy link
Contributor Author

EnricoMi commented Feb 13, 2024

I agree, it should also be removed from https://github.com/apache/incubator-uniffle/blob/d87dc900407d4faa9ba5f80cd19d08513e71082d/.github/workflows/parallel.yml#L87-L91
and added to the matrix profile instead.

Similarly, it should be removed from .github/workflows/sequential.yml and added as a an extra step.

@rickyma
Copy link
Contributor

rickyma commented Feb 14, 2024

Could you continue working on this part when you have some free time? I would greatly appreciate it, this will be very helpful to me. @EnricoMi

@EnricoMi
Copy link
Contributor Author

That just hit me as well, and my changes to build_distribution.s were very ignorant, so here is the fix #1525

@EnricoMi
Copy link
Contributor Author

The CI is fixed in #1526. Please review.

jerqi pushed a commit that referenced this pull request Feb 15, 2024
### What changes were proposed in this pull request?
Improves how dashboard is build and tested in the Github CI.

### Why are the changes needed?
Changes in #1498 were not considering the actual state of dashboard within the Maven project.

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

### How was this patch tested?
CI.
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.

3 participants