[#414] feat(client): support specifying per-partition's max concurrency to write in client side #815

zuston · 2023-04-11T12:52:43Z

What changes were proposed in this pull request?

Support specifying per-partition's max concurrency to write in client side

Why are the changes needed?

The PR of #396 has introduced the concurrent HDFS writing for one partition, but the concurrency is determined by the server client. In order to increase flexibility, this PR supports specifying per-partition's max concurrency to write in client side

Does this PR introduce any user-facing change?

Yes. The client conf of <client_type>.rss.client.max.concurrency.per-partition.write and rss.server.client.max.concurrency.limit.per-partition.write are introduced.

How was this patch tested?

UTs

codecov-commenter · 2023-04-12T02:38:39Z

Codecov Report

Merging #815 (a04bb71) into master (24a5195) will increase coverage by 2.29%.
The diff coverage is 78.94%.

@@             Coverage Diff              @@
##             master     #815      +/-   ##
============================================
+ Coverage     57.13%   59.42%   +2.29%     
- Complexity     2155     2164       +9     
============================================
  Files           322      303      -19     
  Lines         15676    13362    -2314     
  Branches       1246     1247       +1     
============================================
- Hits           8956     7940    -1016     
+ Misses         6213     4987    -1226     
+ Partials        507      435      -72

Impacted Files	Coverage Δ
...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java	`0.00% <0.00%> (ø)`
...he/uniffle/client/impl/ShuffleWriteClientImpl.java	`33.41% <ø> (ø)`
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java	`0.00% <0.00%> (ø)`
...ffle/client/request/RssRegisterShuffleRequest.java	`0.00% <0.00%> (ø)`
...pache/uniffle/server/ShuffleServerGrpcService.java	`0.80% <0.00%> (-0.01%)`	⬇️
...ge/handler/impl/PooledHdfsShuffleWriteHandler.java	`71.42% <ø> (ø)`
...rg/apache/uniffle/common/config/RssClientConf.java	`98.59% <100.00%> (+0.10%)`	⬆️
...org/apache/uniffle/server/ShuffleFlushManager.java	`80.30% <100.00%> (+0.40%)`	⬆️
...a/org/apache/uniffle/server/ShuffleServerConf.java	`99.43% <100.00%> (+<0.01%)`	⬆️
...rg/apache/uniffle/server/ShuffleSpecification.java	`100.00% <100.00%> (ø)`
... and 2 more

... and 22 files with indirect coverage changes

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

server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java

common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java

proto/src/main/proto/Rss.proto

jerqi · 2023-04-17T09:36:24Z

server/src/main/java/org/apache/uniffle/server/ShuffleSpecification.java

+
+import org.apache.uniffle.common.ShuffleDataDistributionType;
+
+public class ShuffleSpecification {


Could we use the builder pattern? Because we will add more fields in the future. This class will vary frequently.

Make sense.

docs/client_guide.md

server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java

jerqi · 2023-04-18T03:12:24Z

server/src/main/java/org/apache/uniffle/server/ShuffleTaskInfo.java

+    return specification.getDistributionType();
+  }
+
+  public void setSpecification(ShuffleDataDistributionType type, int maxConcurrency) {


Could we use setSpecification(ShuffleSpecification spec) here? We don't need synchronized, too.

If so, we also should use the AtomicReference

jerqi

LGTM, Let others take a look.

common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java

advancedxy · 2023-04-18T07:22:33Z

docs/server_guide.md

@@ -89,6 +89,7 @@ This document will introduce how to deploy Uniffle shuffle servers.
 | rss.server.multistorage.fallback.strategy.class       | -       | The fallback strategy for `MEMORY_LOCALFILE_HDFS`. Support `org.apache.uniffle.server.storage.RotateStorageManagerFallbackStrategy`,`org.apache.uniffle.server.storage.LocalStorageManagerFallbackStrategy` and `org.apache.uniffle.server.storage.HdfsStorageManagerFallbackStrategy`. If not set, `org.apache.uniffle.server.storage.HdfsStorageManagerFallbackStrategy` will be used. |
 | rss.server.leak.shuffledata.check.interval            | 3600000 | The interval of leak shuffle data check (ms)                                                                                                                                                                                                                                                                                                                                             |
 | rss.server.max.concurrency.of.single.partition.writer | 1       | The max concurrency of single partition writer, the data partition file number is equal to this value. Default value is 1. This config could improve the writing speed, especially for huge partition.                                                                                                                                                                                   |
+| rss.server.client.max.concurrency.limit.per-partition.write | - | The limit for max concurrency per-partition write specified by client, this won't be enabled by default.                                                                                                                                                                                                                                                                                 |


this doesn't seem right.

It's quite weird to mix rss.server.client here.
Better call it, rss.server.max.concurrency.of.per-partition.write?

description:

Server side limit for max concurrency of per partition writer, which could be specified by client side. If client specifies `rss.client.max.concurrency.per-partition.write`, the `rss.server.max.concurrency.of.single.partition.writer` could be overridden.

this doesn't seem right.

It's quite weird to mix rss.server.client here. Better call it, rss.server.max.concurrency.of.per-partition.write?

description:

Server side limit for max concurrency of per partition writer, which could be specified by client side. If client specifies `rss.client.max.concurrency.per-partition.write`, the `rss.server.max.concurrency.of.single.partition.writer` could be overridden.

+1 . The name is a little weird.

We have rss.server.max.concurrency.of.single.partition.writer. How about rss.server.max.concurrency.limit.of.per-partition.write ?

And I hope rename rss.server.max.concurrency.of.single.partition.writer to rss.server.max.concurrency.of.per-partition.write

It will cause some compatible problems if we rename config option. If you insist on it, you should add migration guide document.

We have rss.server.max.concurrency.of.single.partition.writer. How about rss.server.max.concurrency.limit.of.per-partition.write ?

And I hope rename rss.server.max.concurrency.of.single.partition.writer to rss.server.max.concurrency.of.per-partition.write

That sounds good to me. If you are going to rename. You should deprecate that first in v0.8, new configuration should fall back to that old configuration and maybe remove it from v0.9 or v1.0.

any more update for this?

Yes. I will do but something takes up my time.

And the renaming of rss.server.max.concurrency.of.single.partition.writer to rss.server.max.concurrency.of.per-partition.write will be started after #841 is finished.

jerqi · 2023-04-26T14:42:16Z

We should add some migration documents.

zuston · 2023-04-27T02:03:56Z

We should add some migration documents.

I don't do any incompatible change. The rename config will delay until #841 finish

zuston · 2023-04-27T06:43:09Z

Ping @jerqi

jerqi · 2023-04-27T06:44:32Z

#841 has merged. You can rename the config option and add the migration guide document.

zuston · 2023-04-27T06:48:15Z

#841 has merged. You can rename the config option and add the migration guide document.

I will submit a new PR to do this. This is not related with this PR.

jerqi · 2023-04-27T06:54:17Z

You would better include this change in this pr. It's not big change.

zuston · 2023-04-27T07:02:27Z

You would better include this change in this pr. It's not big change.

OK. I will do.

But I still think any modifications that are not related will need to be done in the different pr.

jerqi · 2023-04-27T07:04:47Z

You would better include this change in this pr. It's not big change.

OK. I will do.

But I still think any modifications that are not related will need to be done in the different pr.

It's the related changes, isn't it? The feature and its configuration option is related.

zuston · 2023-04-27T07:24:12Z

It's the related changes, isn't it? The feature and its configuration option is related.

No. This PR only introduces the conf of rss.server.max.concurrency.limit.of.per-partition.write and rss.client.max.concurrency.of.per-partition.write

But the renaming is for the key of rss.server.max.concurrency.of.single.partition.writer. In this PR, all operations are not related with the concurrency specified by server side

zuston · 2023-04-27T07:31:33Z

Done @jerqi

jerqi

LGTM, thanks @zuston

zuston changed the title ~~Support specifying max concurrency per-partition to write in client side~~ Support specifying per-partition's max concurrency to write in client side Apr 11, 2023

zuston changed the title ~~Support specifying per-partition's max concurrency to write in client side~~ [#414] feat(client): support specifying per-partition's max concurrency to write in client side Apr 12, 2023

advancedxy reviewed Apr 13, 2023

View reviewed changes

server/src/main/java/org/apache/uniffle/server/ShuffleFlushManager.java Show resolved Hide resolved

jerqi reviewed Apr 17, 2023

View reviewed changes

common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java Show resolved Hide resolved

proto/src/main/proto/Rss.proto Show resolved Hide resolved

zuston force-pushed the clientHdfsConfig branch from b57ac1c to 89b2ed9 Compare April 17, 2023 08:07

jerqi reviewed Apr 17, 2023

View reviewed changes

smallzhongfeng reviewed Apr 17, 2023

View reviewed changes

docs/client_guide.md Outdated Show resolved Hide resolved

server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java Show resolved Hide resolved

jerqi reviewed Apr 18, 2023

View reviewed changes

jerqi requested review from advancedxy and smallzhongfeng April 18, 2023 06:12

advancedxy reviewed Apr 18, 2023

View reviewed changes

zuston requested a review from advancedxy April 26, 2023 07:38

smallzhongfeng previously approved these changes Apr 26, 2023

View reviewed changes

zuston added 8 commits April 27, 2023 15:03

Support specifying max concurrency per-partition to write in client side

5329c56

fix

43f5e54

update doc

42aebf5

fix

4490a28

Introducing shuffle specification

8dc4e38

add max concurrency limit

0beefc1

fix

3f20182

fix

1266c6e

zuston added 4 commits April 27, 2023 15:03

builfer

1fde873

doc

4d73027

fix

1ab3d99

rename to rss.server.max.concurrency.limit.of.per-partition.write

2662cbb

change keys

a04bb71

zuston dismissed smallzhongfeng’s stale review via a04bb71 April 27, 2023 07:31

zuston force-pushed the clientHdfsConfig branch from 8c155da to a04bb71 Compare April 27, 2023 07:31

jerqi approved these changes Apr 27, 2023

View reviewed changes

zuston merged commit 8edefdf into apache:master Apr 27, 2023

zuston mentioned this pull request Apr 28, 2023

[FEATURE] Support max concurrency specified by client side in HDFS storage #414

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#414] feat(client): support specifying per-partition's max concurrency to write in client side #815

[#414] feat(client): support specifying per-partition's max concurrency to write in client side #815

zuston commented Apr 11, 2023 •

edited

Loading

codecov-commenter commented Apr 12, 2023 •

edited

Loading

jerqi Apr 17, 2023

zuston Apr 17, 2023

jerqi Apr 18, 2023

zuston Apr 18, 2023

zuston Apr 18, 2023

zuston Apr 18, 2023

jerqi left a comment

advancedxy Apr 18, 2023

jerqi Apr 18, 2023

zuston Apr 18, 2023

jerqi Apr 18, 2023

advancedxy Apr 20, 2023

advancedxy Apr 23, 2023

zuston Apr 23, 2023

zuston Apr 26, 2023

zuston Apr 26, 2023

jerqi commented Apr 26, 2023

zuston commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi commented Apr 27, 2023

zuston commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi left a comment


		import org.apache.uniffle.common.ShuffleDataDistributionType;

		public class ShuffleSpecification {

[#414] feat(client): support specifying per-partition's max concurrency to write in client side #815

[#414] feat(client): support specifying per-partition's max concurrency to write in client side #815

Conversation

zuston commented Apr 11, 2023 • edited Loading

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 Apr 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Apr 26, 2023

zuston commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi commented Apr 27, 2023

zuston commented Apr 27, 2023

zuston commented Apr 27, 2023

jerqi left a comment

Choose a reason for hiding this comment

zuston commented Apr 11, 2023 •

edited

Loading

codecov-commenter commented Apr 12, 2023 •

edited

Loading