[#960] fix(dashboard): simplify dependency and correct the startup script by yl09099 · Pull Request #1347 · apache/uniffle

yl09099 · 2023-12-05T08:20:44Z

What changes were proposed in this pull request?

simplify dependency and correct the startup script.

Why are the changes needed?

(Please clarify why the changes are needed. For instance,

Fix: #960

Does this PR introduce any user-facing change?

(Please list the user-facing changes introduced by your change, including

No.

How was this patch tested?

Needn't.

roryqi · 2023-12-05T08:58:10Z

What's this pr the relationship of #1326

yl09099 · 2023-12-05T09:40:04Z

#1326

Some fixes based on PR #1326.I'm making changes after the merger.

yl09099 · 2023-12-11T07:15:21Z

@zuston @xianjingfeng @jerqi Modify the dashboard pom dependencies, modify the startup script to be independent of Hadoop, and separate the dashboard configuration file from the Coordinator configuration file so that it can be deployed independently.

codecov-commenter · 2023-12-11T07:19:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ecbf2e7) 53.22% compared to head (1d8942d) 54.13%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1347      +/-   ##
============================================
+ Coverage     53.22%   54.13%   +0.90%     
  Complexity     2715     2715              
============================================
  Files           418      398      -20     
  Lines         23932    21571    -2361     
  Branches       2043     2043              
============================================
- Hits          12738    11677    -1061     
+ Misses        10408     9180    -1228     
+ Partials        786      714      -72

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zuston

LGTM.

xianjingfeng · 2023-12-11T08:18:48Z

Could you fix these two problems in this pr? @yl09099
#1339 (comment)

yl09099 · 2023-12-11T08:23:08Z

Could you fix these two problems in this pr? @yl09099 #1339 (comment)

This has been modified.

xianjingfeng · 2023-12-11T08:33:15Z

Could you fix these two problems in this pr? @yl09099 #1339 (comment)

This has been modified.

It seems that one of them was fixed. But we still must set the env HADOOP_HOME, right?

yl09099 · 2023-12-11T09:09:32Z

Could you fix these two problems in this pr? @yl09099 #1339 (comment)

This has been modified.

It seems that one of them was fixed. But we still must set the env HADOOP_HOME, right?

I have deleted the HDFS configuration in start-dashboard and do not need to set HADOOP_HOME on dashboard.

xianjingfeng · 2023-12-11T09:17:03Z

Could you fix these two problems in this pr? @yl09099 #1339 (comment)

This has been modified.

It seems that one of them was fixed. But we still must set the env HADOOP_HOME, right?

I have deleted the HDFS configuration in start-dashboard and do not need to set HADOOP_HOME on dashboard.

Is there anything problems with the following code?
https://github.com/apache/incubator-uniffle/blob/ecbf2e79423505dd44bf54c612572c5c9da71175/bin/utils.sh#L175-L178

yl09099 · 2023-12-12T03:53:04Z

Could you fix these two problems in this pr? @yl09099 #1339 (comment)

This has been modified.

It seems that one of them was fixed. But we still must set the env HADOOP_HOME, right?

I have deleted the HDFS configuration in start-dashboard and do not need to set HADOOP_HOME on dashboard.

Is there anything problems with the following code?

https://github.com/apache/incubator-uniffle/blob/ecbf2e79423505dd44bf54c612572c5c9da71175/bin/utils.sh#L175-L178

Learned, changed

xianjingfeng · 2023-12-12T09:57:40Z

+  if [[ $is_dashboard -eq 0 || -z "$HADOOP_HOME" ]]; then
+    echo "Dashboard need not HADOOP_HOME."
+  elif [[ -z "$HADOOP_HOME" ]]; then
    echo "No env HADOOP_HOME."
    exit 1
  fi


Suggested change

if [[ $is_dashboard -eq 0 || -z "$HADOOP_HOME" ]]; then

echo "Dashboard need not HADOOP_HOME."

elif [[ -z "$HADOOP_HOME" ]]; then

echo "No env HADOOP_HOME."

exit 1

fi

if [[ -z "$HADOOP_HOME" ]]; then

if [[ $is_dashboard -eq 1 ]]; then

echo "Dashboard need not HADOOP_HOME."

else

echo "No env HADOOP_HOME."

exit 1

fi

fi

xianjingfeng · 2023-12-12T10:00:07Z

+  is_dashboard=1
+  if [ $# -eq 1 ]; then
+          is_dashboard=$1
+  fi


Suggested change

is_dashboard=1

if [ $# -eq 1 ]; then

is_dashboard=$1

fi

is_dashboard=0

if [ $# -eq 1 ]; then

is_dashboard=1

fi

@zuston @jerqi Do you have any idea about this part?

Have no better idea. Follow your suggestion.

…tup script.

xianjingfeng

LGTM

zuston · 2023-12-13T09:44:18Z

Thanks @yl09099 @xianjingfeng

* master: (23 commits) format use synchronized format modify to singleton format format format format format change Coordinator to singleton like ShuffleServerClientFactory.java [apache#1189] feat(server): Add netty used direct memory size metric (apache#1363) format format format rename file rename [apache#960] fix(dashboard): simplify dependency and correct the startup script (apache#1347) [apache#1348] improvement(metrics): Unify tags generation for shuffle-server metrics reporter (apache#1349) [MINOR] chore: fix kubernetes ci pipeline (apache#1368) [MINOR] fix(spark): Fix NPE for ShuffleWriteClientImpl.unregisterShuffle (apache#1367) ...

…re_1119_new_2023_12_23 * feature_1119_BUFFER_LIMIT_OF_HUGE_PARTITION: add file for commit [apache#1189] feat(server): Add netty used direct memory size metric (apache#1363) format format format rename file rename [apache#960] fix(dashboard): simplify dependency and correct the startup script (apache#1347) [apache#1348] improvement(metrics): Unify tags generation for shuffle-server metrics reporter (apache#1349) [MINOR] chore: fix kubernetes ci pipeline (apache#1368) [MINOR] fix(spark): Fix NPE for ShuffleWriteClientImpl.unregisterShuffle (apache#1367) format code format code 添加堆外内存监控的代码

…tup script (apache#1347) ### What changes were proposed in this pull request? simplify dependency and correct the startup script. ### Why are the changes needed? Fix: apache#960 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Needn't.

yl09099 changed the title ~~Uniffle 960 5~~ [#960] fix(dashboard): simplify dependency and correct the startup script Dec 5, 2023

roryqi requested a review from xianjingfeng December 5, 2023 08:33

yl09099 force-pushed the uniffle-960-5 branch from 1bca0ad to 0eb1ac8 Compare December 11, 2023 07:09

zuston previously approved these changes Dec 11, 2023

View reviewed changes

yl09099 dismissed zuston’s stale review via 0710fad December 12, 2023 03:52

yl09099 force-pushed the uniffle-960-5 branch from 0eb1ac8 to 0710fad Compare December 12, 2023 03:52

yl09099 force-pushed the uniffle-960-5 branch 2 times, most recently from db54e97 to de7a153 Compare December 12, 2023 03:55

xianjingfeng reviewed Dec 12, 2023

View reviewed changes

[apache#960] fix(dashboard): simplify dependency and correct the star…

1d8942d

…tup script.

yl09099 force-pushed the uniffle-960-5 branch from de7a153 to 1d8942d Compare December 13, 2023 03:21

yl09099 requested review from xianjingfeng and zuston December 13, 2023 03:23

xianjingfeng approved these changes Dec 13, 2023

View reviewed changes

zuston merged commit 3b0cbb9 into apache:master Dec 13, 2023

Uh oh!

Conversation

yl09099 commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

roryqi commented Dec 5, 2023

Uh oh!

yl09099 commented Dec 5, 2023

Uh oh!

yl09099 commented Dec 11, 2023

Uh oh!

codecov-commenter commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

xianjingfeng commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yl09099 commented Dec 11, 2023

Uh oh!

xianjingfeng commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yl09099 commented Dec 11, 2023

Uh oh!

xianjingfeng commented Dec 11, 2023

Uh oh!

yl09099 commented Dec 12, 2023

Uh oh!

xianjingfeng Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xianjingfeng Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

xianjingfeng Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

roryqi Dec 13, 2023

Choose a reason for hiding this comment

Uh oh!

xianjingfeng left a comment

Choose a reason for hiding this comment

Uh oh!

zuston commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yl09099 commented Dec 5, 2023 •

edited

Loading

codecov-commenter commented Dec 11, 2023 •

edited

Loading

xianjingfeng commented Dec 11, 2023 •

edited

Loading

xianjingfeng commented Dec 11, 2023 •

edited

Loading

xianjingfeng Dec 12, 2023 •

edited

Loading