Use the conf of shuffleNodesNumber from jobs to be as checking factor #208

zuston · 2022-09-09T07:08:39Z

What changes were proposed in this pull request?

Use the conf of shuffleNodesNumber from jobs to be as checking factor

Why are the changes needed?

In the PR #97 , it allow client to specify the shuffle server numbers, but in clusterLoaderChecker, it dont take this into considering. In this PR, it will use the conf of shuffleNodesNumber from jobs to be as checking factor only when the conf of rss.coordinator.access.loadChecker.serverNum.threshold is missed.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UTs.

codecov-commenter · 2022-09-09T07:33:03Z

Codecov Report

Merging #208 (240fb1a) into master (9b83f66) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #208      +/-   ##
============================================
+ Coverage     59.02%   59.17%   +0.14%     
- Complexity     1326     1332       +6     
============================================
  Files           160      160              
  Lines          8699     8732      +33     
  Branches        815      819       +4     
============================================
+ Hits           5135     5167      +32     
+ Misses         3301     3300       -1     
- Partials        263      265       +2

Impacted Files	Coverage Δ
...java/org/apache/uniffle/common/util/Constants.java	`0.00% <ø> (ø)`
.../uniffle/coordinator/AccessClusterLoadChecker.java	`96.96% <100.00%> (+0.81%)`	⬆️
...rg/apache/uniffle/coordinator/CoordinatorConf.java	`96.94% <100.00%> (ø)`
...orage/handler/impl/LocalFileServerReadHandler.java	`77.96% <0.00%> (-2.40%)`	⬇️
.../java/org/apache/uniffle/common/util/RssUtils.java	`69.32% <0.00%> (-0.80%)`	⬇️
.../org/apache/spark/shuffle/writer/WriterBuffer.java	`93.18% <0.00%> (-0.30%)`	⬇️
.../storage/handler/impl/HdfsShuffleWriteHandler.java	`85.07% <0.00%> (ø)`
...rg/apache/uniffle/server/ShuffleServerMetrics.java	`96.11% <0.00%> (+0.24%)`	⬆️
...in/java/org/apache/uniffle/server/HealthCheck.java	`61.36% <0.00%> (+1.83%)`	⬆️
...org/apache/uniffle/server/LocalStorageChecker.java	`70.19% <0.00%> (+2.45%)`	⬆️
... and 3 more

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

zuston · 2022-09-09T07:57:29Z

PTAL @jerqi

jerqi · 2022-09-10T14:11:18Z

client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java

@@ -99,13 +103,17 @@ private boolean tryAccessCluster() {
    long retryInterval = sparkConf.get(RssSparkConfig.RSS_CLIENT_ACCESS_RETRY_INTERVAL_MS);
    int retryTimes = sparkConf.get(RssSparkConfig.RSS_CLIENT_ACCESS_RETRY_TIMES);

+    int assignmentShuffleNodesNum = sparkConf.get(RssSparkConfig.RSS_CLIENT_ASSIGNMENT_SHUFFLE_SERVER_NUMBER);


Why do we only modify spark3?

Sorry, I forgot to modify in spark2

common/src/main/java/org/apache/uniffle/common/util/Constants.java

jerqi · 2022-09-13T02:42:01Z

RSS_CLIENT_ASSIGNMENT_SHUFFLE_SERVER_NUMBER means that the max number should be allocated for client. It's not min number that should be allocated for client. I think it's not suitable to judge whether to reject the application.

zuston · 2022-09-13T04:02:38Z

The conf of rss.coordinator.shuffle.nodes.max in coordinator is a soft limitation. It means when available nodes number is lower than this value, it will not throw exception and reassign the number of all available nodes.

But in current implementation of AccessClusterLoadChecker, it will reject the app when available nodes number < rss.coordinator.shuffle.nodes.max.

For example

available shuffle nodes number = 20
job required shuffle nodes number = 10
rss.coordinator.shuffle.nodes.max = 30

The AccessClusterLoadChecker will reject this job. I think this is unreasonable.

jerqi · 2022-09-13T07:38:48Z

The conf of rss.coordinator.shuffle.nodes.max in coordinator is a soft limitation. It means when available nodes number is lower than this value, it will not throw exception and reassign the number of all available nodes.

But in current implementation of AccessClusterLoadChecker, it will reject the app when available nodes number < rss.coordinator.shuffle.nodes.max.

For example

available shuffle nodes number = 20 job required shuffle nodes number = 10 rss.coordinator.shuffle.nodes.max = 30

The AccessClusterLoadChecker will reject this job. I think this is unreasonable.

I got it.

jerqi · 2022-09-13T07:46:48Z

But the rss.coordinator.shuffle.nodes.max is just the default value. If you have other need, you can change this default value. rss.coordinator.access.loadChecker.serverNum.threshold isn't related the to rss.coordinator.shuffle.nodes.max actually.

zuston · 2022-09-13T09:10:38Z

You mean we should remove the check of rss.coordinator.shuffle.nodes.max in AccessClusterLoadChecker?

jerqi · 2022-09-14T03:22:14Z

You mean we should remove the check of rss.coordinator.shuffle.nodes.max in AccessClusterLoadChecker?

Origin logic don't check rss.coordinator.shuffle.nodes.max. It's a config option rss.coordinator.access.loadChecker.serverNum.threshold. Its default value is rss.coordinator.shuffle.nodes.max. Default value don't need to be reasonable for every situation.

zuston · 2022-09-14T03:58:51Z

Got your thought.

But the conf of rss.coordinator.access.loadChecker.serverNum.threshold is a hard limitation, if value is too low, it will pass all. If too high, it will reject all.

For the equality of every job, I wont set it and hope the checker can decide whether to reject depending on the required shuffle servers number of jobs.

I thinks this is reasonable, it will still reserve the hard limitation and keep the flexibility

jerqi · 2022-09-14T04:03:22Z

Got your thought.

But the conf of rss.coordinator.access.loadChecker.serverNum.threshold is a hard limitation, if value is too low, it will pass all. If too high, it will reject all.

For the equality of every job, I wont set it and hope the checker can decide whether to reject depending on the required shuffle servers number of jobs.

I thinks this is reasonable, it will still reserve the hard limitation and keep the flexibility

Should we give a option of coordinator to decide whether to use the required shuffle server of client for users?

zuston · 2022-09-14T05:05:36Z

Should we give a option of coordinator to decide whether to use the required shuffle server of client for users?

Not necessary. If the conf value of rss.client.assignment.shuffle.nodes.max is -1 or not set, it will use the conf from coordinator side.

jerqi · 2022-09-14T08:36:05Z

Should we give a option of coordinator to decide whether to use the required shuffle server of client for users?

Not necessary. If the conf value of rss.client.assignment.shuffle.nodes.max is -1 or not set, it will use the conf from coordinator side.

If I want to use rss.coordinator.access.loadChecker.serverNum.threshold, it's weird that we must guarantee that the conf value of rss.client.assignment.shuffle.nodes.max is -1 or not set. They are not related config options.

zuston · 2022-09-14T08:54:42Z

Oh. Sorry for not explaining clearly.

If the rss.coordinator.access.loadChecker.serverNum.threshold is set, this checker will only use this value as the checking condition.
If the rss.coordinator.access.loadChecker.serverNum.threshold is missing, this checker will use the client's shuffle-servers number as checking condition. If the value of ss.client.assignment.shuffle.nodes.max is -1 or not set, then it will fallback to use the coodinator side's rss.coordinator.shuffle.nodes.max

jerqi · 2022-09-14T10:49:15Z

Like discussion at dev mail list, we will freeze the code and cut 0.6 version branch in September 15, we will not merge this pr before I cut 0.6 version branch, are you ok?

zuston · 2022-09-14T12:56:48Z

Like discussion at dev mail list, we will freeze the code and cut 0.6 version branch in September 15, we will not merge this pr before I cut 0.6 version branch, are you ok?

Got it

zuston · 2022-09-19T08:01:36Z

Ping @jerqi

jerqi · 2022-09-19T10:53:33Z

Should we add more documents for this feature?

zuston · 2022-09-19T11:23:05Z

Should we add more documents for this feature?

Added.

jerqi

LGTM, thanks @zuston

zuston · 2022-09-20T02:40:14Z

Thanks for your patient review @jerqi

Use the conf of shuffleNodesNumber from jobs to be as checking factor

e8f7ce1

fix

5cdbf0f

jerqi reviewed Sep 10, 2022

View reviewed changes

Support for spark2

b55d2e3

doc

240fb1a

zuston requested a review from jerqi September 20, 2022 02:03

jerqi approved these changes Sep 20, 2022

View reviewed changes

jerqi merged commit 9dc7f0d into apache:master Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the conf of shuffleNodesNumber from jobs to be as checking factor #208

Use the conf of shuffleNodesNumber from jobs to be as checking factor #208

zuston commented Sep 9, 2022

codecov-commenter commented Sep 9, 2022 •

edited

Loading

zuston commented Sep 9, 2022

jerqi Sep 10, 2022

zuston Sep 10, 2022

jerqi commented Sep 13, 2022

zuston commented Sep 13, 2022

jerqi commented Sep 13, 2022

For example

jerqi commented Sep 13, 2022

zuston commented Sep 13, 2022

jerqi commented Sep 14, 2022

zuston commented Sep 14, 2022

jerqi commented Sep 14, 2022 •

edited

Loading

zuston commented Sep 14, 2022

jerqi commented Sep 14, 2022

zuston commented Sep 14, 2022

jerqi commented Sep 14, 2022

zuston commented Sep 14, 2022

zuston commented Sep 19, 2022

jerqi commented Sep 19, 2022

zuston commented Sep 19, 2022

jerqi left a comment

zuston commented Sep 20, 2022

Use the conf of shuffleNodesNumber from jobs to be as checking factor #208

Use the conf of shuffleNodesNumber from jobs to be as checking factor #208

Conversation

zuston commented Sep 9, 2022

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?

codecov-commenter commented Sep 9, 2022 • edited Loading

Codecov Report

zuston commented Sep 9, 2022

jerqi Sep 10, 2022

Choose a reason for hiding this comment

zuston Sep 10, 2022

Choose a reason for hiding this comment

jerqi commented Sep 13, 2022

zuston commented Sep 13, 2022

For example

jerqi commented Sep 13, 2022

For example

jerqi commented Sep 13, 2022

zuston commented Sep 13, 2022

jerqi commented Sep 14, 2022

zuston commented Sep 14, 2022

jerqi commented Sep 14, 2022 • edited Loading

zuston commented Sep 14, 2022

jerqi commented Sep 14, 2022

zuston commented Sep 14, 2022

jerqi commented Sep 14, 2022

zuston commented Sep 14, 2022

zuston commented Sep 19, 2022

jerqi commented Sep 19, 2022

zuston commented Sep 19, 2022

jerqi left a comment

Choose a reason for hiding this comment

zuston commented Sep 20, 2022

codecov-commenter commented Sep 9, 2022 •

edited

Loading

jerqi commented Sep 14, 2022 •

edited

Loading