[#1303] refactor(client): using the configOption in RssTezConfig #1413

lifeSo · 2024-01-01T17:57:55Z

What changes were proposed in this pull request?

Improvement: using the configOption in RssTezConfig #1303

Why are the changes needed?

Fix: #1303

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test

codecov-commenter · 2024-01-01T18:11:50Z

Codecov Report

Attention: 79 lines in your changes are missing coverage. Please review.

Comparison is base (15a399a) 53.43% compared to head (79a08b2) 54.88%.
Report is 4 commits behind head on master.

Files	Patch %	Lines
...main/java/org/apache/tez/common/TezClientConf.java	95.57%	11 Missing and 2 partials ⚠️
...java/org/apache/uniffle/common/config/RssConf.java	0.00%	13 Missing ⚠️
...c/main/java/org/apache/tez/common/RssTezUtils.java	78.57%	5 Missing and 4 partials ⚠️
...n/java/org/apache/tez/dag/app/RssDAGAppMaster.java	77.77%	7 Missing and 1 partial ⚠️
...library/common/shuffle/impl/RssTezFetcherTask.java	0.00%	8 Missing ⚠️
...tez/runtime/library/input/RssUnorderedKVInput.java	0.00%	8 Missing ⚠️
.../org/apache/uniffle/common/config/RssBaseConf.java	0.00%	5 Missing ⚠️
...rg/apache/tez/dag/app/TezRemoteShuffleManager.java	60.00%	3 Missing and 1 partial ⚠️
...rary/common/shuffle/orderedgrouped/RssShuffle.java	50.00%	4 Missing ⚠️
...on/shuffle/orderedgrouped/RssShuffleScheduler.java	72.72%	3 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1413      +/-   ##
============================================
+ Coverage     53.43%   54.88%   +1.45%     
- Complexity     2723     2737      +14     
============================================
  Files           419      402      -17     
  Lines         23992    22015    -1977     
  Branches       2046     2063      +17     
============================================
- Hits          12820    12083     -737     
+ Misses        10378     9201    -1177     
+ Partials        794      731      -63

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

zuston · 2024-01-02T10:07:08Z

I like this PR that refactor the client config. Could you help fix the failed test cases

zuston · 2024-01-02T10:07:46Z

Could you help review this? @zhengchenyu

lifeSo · 2024-01-06T05:41:21Z

I like this PR that refactor the client config. Could you help fix the failed test cases

Ok, I will fix the failed test cases recently.

jerqi · 2024-01-08T07:27:09Z

ping @zhengchenyu

client-tez/src/test/java/org/apache/tez/common/TezClientConfTest.java

zhengchenyu · 2024-01-08T13:52:30Z

client-tez/src/test/java/org/apache/tez/dag/app/TezRemoteShuffleManagerTest.java

-              appId.toString(), sessionToken, conf, appId.toString(), client, null);
+              appId.toString(),
+              sessionToken,
+              new TezClientConf(conf),


Can we construct TezClientConf directly? Then we will ignore conversion process!

I think there needs a way to init and read configuration, like ShuffleServerConf 、DashboardConf、CoordinatorConf read config from fileName:

public ShuffleServerConf(String fileName) { super(); boolean ret = loadConfFromFile(fileName); if (!ret) { throw new IllegalStateException("Fail to load config file " + fileName); } }

public DashboardConf(String fileName) { super(); boolean ret = loadConfFromFile(fileName); if (!ret) { throw new IllegalStateException("Fail to load config file " + fileName); } }

public CoordinatorConf(String fileName) { super(); boolean ret = loadConfFromFile(fileName); if (!ret) { throw new IllegalStateException("Fail to load config file " + fileName); } }

And the only difference is TezClientConf init from configuration。

I means that new TezClientConf() without any parameter, then set TEZ_RSS_TEST_MODE_ENABLE.
Then TezClientConf without any parameter will be zero cost.
Here we must load configuration, I think it is not neccessary. Because the configuration in TezClientConf(new Configuration) doesn't pass any useful config.

I think set TEZ_RSS_TEST_MODE_ENABLE should be obvious, and not like hidden in TezClientConf().

The normal case should read config from client side by Configuration, and convert to TezClientConfig, so there need TezClientConfig(new Configuration()).
And no need new TezClientConfig(), because if so, there will be default config and meanless.

So, I changed the test case to be:TezClientConf(new Configuration()), new Configuration() just like conf from client side.

client-tez/src/main/java/org/apache/tez/common/RssTezUtils.java

client-tez/src/main/java/org/apache/tez/common/TezClientConf.java

zhengchenyu · 2024-01-09T02:13:31Z

client-tez/src/main/java/org/apache/tez/common/TezClientConf.java

+    return loadConfFromHadoopConfig(config, ConfigUtils.getAllConfigOptions(TezClientConf.class));
+  }
+
+  public RssConf toRssConf() {


TezClientConf is extended from RssBaseConf, RssBaseConf is extended from RssConf. So is this method still necessary?

It is necessary.

public RssConf toRssConf() { RssConf rssConf = new RssConf(); for (Map.Entry<String, Object> entry : getAll()) { String key = entry.getKey(); if (!key.startsWith(TEZ_RSS_CONFIG_PREFIX)) { continue; } key = key.substring(TEZ_RSS_CONFIG_PREFIX.length()); rssConf.setString(key, String.valueOf(entry.getValue())); } return rssConf; }

Between the conversion, to RssConf, removed prefex. like:

key = key.substring(TEZ_RSS_CONFIG_PREFIX.length()); rssConf.setString(key, String.valueOf(entry.getValue()));

I see you!
But I don't know the design of these issue. How about RssClientConfig?
Maybe RssClientConfig should be extended from RssConf or RssBaseConf. RssTezConfig should be extended from RssClientConfig.
The the config key in RssXXXConfig should not be start with "tez", and the config key in hadoop Configuration should be start with "tez". That's my understanding, maybe it is wrong.

What do you think? @lifeSo

@zuston Can you give us some advice?

In now design, the config in Tez client, start with "tez.",
similar, the config in Spark client, start with "spark.".
And there is also leading substring in RssSparkConfig:

public static RssConf toRssConf(SparkConf sparkConf) { RssConf rssConf = new RssConf(); for (Tuple2<String, String> tuple : sparkConf.getAll()) { String key = tuple._1; if (!key.startsWith(SPARK_RSS_CONFIG_PREFIX)) { continue; } key = key.substring(SPARK_RSS_CONFIG_PREFIX.length()); rssConf.setString(key, tuple._2); } return rssConf; }

If some tez config not start with "tez.", it will be confusing because some default tez config is start with 'tez.'.

eg:

https://github.com/apache/tez/blob/master/tez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java

zhengchenyu · 2024-01-09T02:14:13Z

...-tez/src/main/java/org/apache/tez/runtime/library/common/shuffle/impl/RssTezFetcherTask.java

@@ -195,15 +190,15 @@ protected FetchResult callInternal() throws Exception {
                      .hadoopConf(hadoopConf)
                      .idHelper(new TezIdHelper())
                      .expectedTaskIdsBitmapFilterEnable(expectedTaskIdsBitmapFilterEnable)
-                      .rssConf(RssTezConfig.toRssConf(this.conf)));
+                      .rssConf(this.conf.toRssConf()));


Same with toRssConf. Can we pass this.conf directly?

I think not, the RSS_STORAGE_TYPE in TezClientConf is 'tez.rss.storage.type',

public static final ConfigOption<String> RSS_STORAGE_TYPE = ConfigOptions.key("tez.rss.storage.type") .stringType() .defaultValue("MEMORY_LOCALFILE") .withDescription("");

But in ShuffleReadClientImpl.java, when get storageType, it call:
final String storageType = builder.getRssConf().get(RssClientConf.RSS_STORAGE_TYPE);

The problem is RssClientConf.RSS_STORAGE_TYPE is:

public static final ConfigOption<String> RSS_STORAGE_TYPE = ConfigOptions.key("rss.storage.type") .stringType() .defaultValue("") .withDescription("Supports MEMORY_LOCALFILE, MEMORY_HDFS, MEMORY_LOCALFILE_HDFS");

There is no prefix : 'tez.' .

So, there need the conversion to delete prefix: 'tez'.

zhengchenyu · 2024-01-09T02:27:08Z

It is a great PR! The code about config will be cleaner!
But I've always had a question. There are a lot of transitions from Configuration to RssConf, then from RssConf to Configuration. It seems strange.
In this PR, I think @lifeSo used cache Configuration in TezClientConf, it is more efficient, but additional memory is required. @zuston What do you think of this approach?

zuston · 2024-01-09T03:30:27Z

In this PR, I think @lifeSo used cache Configuration in TezClientConf, it is more efficient, but additional memory is required.

LGTM. It looks the memory will not consume much more. If I'm wrong, feel free to correct.

lifeSo · 2024-01-09T19:58:51Z

I summarized the usage of TezClientConf, and why need the transitions:

GetShuffleServerRequest & GetShuffleServerResponse & IdUtils & InputContextUtils
no nedd conf
RssDAGAppMaster
need TezClientConf to get arguement and transitions.
TezRemoteShuffleManager
need TezClientConf and configuration ，to get arguement and transitions.
RssTezUtils
need RssTezUtils
ShuffleAssignmentsInfoWritable
no need conf
TezClassLoader
no need conf
UmbilicalUtils
need configuration to pass arguement
RssTezAMPolicyProvider
no conf
RemoteFetchedInput
no conf
RssShuffleManager
no need TezClientConf to get conf but transitions is needed.
RssSimpleFetchedInputAllocator
need configuration and TezClientConf to get some conf
RssTezFetcher
no need configuration and TezClientConf
RssTezFetcherTask
need TezClientConf to get argue，also need Configuration to get transitions.
RssInMemoryMerger & RssMergeManager & RssShuffle & RssShuffleScheduler
need configuration to call other Tez class, no need TezClientConf
RssTezBypassWriter & RssTezShuffleDataFetcher & WriteBuffer
WriteBufferManager
no need , just pass RssConf.
RssSorter & RssUnSorter
need Configuration and use only in super()
RssTezPerPartitionRecord
no conf
RssOrderedPartitionedKVOutput & RssUnorderedKVOutput & RssUnorderedPartitionedKVOutput
no nedd TezClientConf.

zuston · 2024-01-12T02:03:12Z

cc @zhengchenyu I'm not familiar with Tez code.

zhengchenyu · 2024-01-12T02:08:18Z

cc @zhengchenyu I'm not familiar with Tez code.

This PR does not involve the core logic of Tez. This PR is only about how the MR/Tez/Spark client uses configuration.
I'm still a little confused about how to design the client config.

zuston

Emm... I take a look about current impl, I don't think we should take the tez prefix into the config key, you can refer the spark's RssClientConf .

In spark's partial configOption, we just extract the spark's conf like spark.rss.client.type into RssConf, that means we could share some general configOption defined in the RssClientConf. So you just only create the TezClientConfig.java, which could define some tez specified configOptions, shareable configs could use defined in RssClientConf. cc @zhengchenyu

zhengchenyu · 2024-01-12T06:47:31Z

Emm... I take a look about current impl, I don't think we should take the tez prefix into the config key, you can refer the spark's RssClientConf .

In spark's partial configOption, we just extract the spark's conf like spark.rss.client.type into RssConf, that means we could share some general configOption defined in the RssClientConf. So you just only create the TezClientConfig.java, which could define some tez specified configOptions, shareable configs could use defined in RssClientConf. cc @zhengchenyu

@zuston I agree with you!
If so, RssClientConf should be extended from RssClientConf (But for now, RssClientConf is extended from RssBaseConf ), and does not take the tez prefix into the config key.
I think the tez prefix should be considered in the TezConfiguration. @lifeSo what about you?

lifeSo changed the title ~~[#1303] Improvement: using the configOption in RssTezConfig #1303~~ [#1303] refactor(client): using the configOption in RssTezConfig #1303 Jan 1, 2024

lifeSo force-pushed the feature_1303_configuration branch 2 times, most recently from d1b1f26 to 529c34c Compare January 2, 2024 03:25

zuston requested a review from zhengchenyu January 2, 2024 10:07

lifeSo force-pushed the feature_1303_configuration branch 10 times, most recently from 1869dd1 to 7498984 Compare January 8, 2024 06:49

lifeSo changed the title ~~[#1303] refactor(client): using the configOption in RssTezConfig #1303~~ [#1303] refactor(client): using the configOption in RssTezConfig Jan 8, 2024

zhengchenyu reviewed Jan 9, 2024

View reviewed changes

lifeSo force-pushed the feature_1303_configuration branch 2 times, most recently from 7fc7d6d to 79a08b2 Compare January 9, 2024 19:58

lifeSo force-pushed the feature_1303_configuration branch from 79a08b2 to c1bb5bc Compare January 10, 2024 15:43

lifeSo requested review from zhengchenyu and jerqi January 11, 2024 07:49

modify TezRemoteShuffleManager config to TezClientConfig

368e632

lifeSo force-pushed the feature_1303_configuration branch from c1bb5bc to 368e632 Compare January 11, 2024 18:58

zuston reviewed Jan 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1303] refactor(client): using the configOption in RssTezConfig #1413

[#1303] refactor(client): using the configOption in RssTezConfig #1413

lifeSo commented Jan 1, 2024

codecov-commenter commented Jan 1, 2024 •

edited

zuston commented Jan 2, 2024

zuston commented Jan 2, 2024

lifeSo commented Jan 6, 2024

jerqi commented Jan 8, 2024

zhengchenyu Jan 8, 2024

lifeSo Jan 9, 2024 •

edited

zhengchenyu Jan 11, 2024

lifeSo Jan 11, 2024 •

edited

zhengchenyu Jan 9, 2024

lifeSo Jan 9, 2024 •

edited

zhengchenyu Jan 11, 2024

lifeSo Jan 11, 2024

lifeSo Jan 11, 2024

zhengchenyu Jan 9, 2024

lifeSo Jan 9, 2024 •

edited

zhengchenyu commented Jan 9, 2024

zuston commented Jan 9, 2024 •

edited

lifeSo commented Jan 9, 2024 •

edited

zuston commented Jan 12, 2024

zhengchenyu commented Jan 12, 2024

zuston left a comment •

edited

zhengchenyu commented Jan 12, 2024 •

edited

[#1303] refactor(client): using the configOption in RssTezConfig #1413

Are you sure you want to change the base?

[#1303] refactor(client): using the configOption in RssTezConfig #1413

Conversation

lifeSo commented Jan 1, 2024

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 Jan 1, 2024 • edited

Codecov Report

zuston commented Jan 2, 2024

zuston commented Jan 2, 2024

lifeSo commented Jan 6, 2024

jerqi commented Jan 8, 2024

zhengchenyu Jan 8, 2024

Choose a reason for hiding this comment

lifeSo Jan 9, 2024 • edited

Choose a reason for hiding this comment

zhengchenyu Jan 11, 2024

Choose a reason for hiding this comment

lifeSo Jan 11, 2024 • edited

Choose a reason for hiding this comment

zhengchenyu Jan 9, 2024

Choose a reason for hiding this comment

lifeSo Jan 9, 2024 • edited

Choose a reason for hiding this comment

zhengchenyu Jan 11, 2024

Choose a reason for hiding this comment

lifeSo Jan 11, 2024

Choose a reason for hiding this comment

lifeSo Jan 11, 2024

Choose a reason for hiding this comment

zhengchenyu Jan 9, 2024

Choose a reason for hiding this comment

lifeSo Jan 9, 2024 • edited

Choose a reason for hiding this comment

zhengchenyu commented Jan 9, 2024

zuston commented Jan 9, 2024 • edited

lifeSo commented Jan 9, 2024 • edited

zuston commented Jan 12, 2024

zhengchenyu commented Jan 12, 2024

zuston left a comment • edited

Choose a reason for hiding this comment

zhengchenyu commented Jan 12, 2024 • edited

codecov-commenter commented Jan 1, 2024 •

edited

lifeSo Jan 9, 2024 •

edited

lifeSo Jan 11, 2024 •

edited

lifeSo Jan 9, 2024 •

edited

lifeSo Jan 9, 2024 •

edited

zuston commented Jan 9, 2024 •

edited

lifeSo commented Jan 9, 2024 •

edited

zuston left a comment •

edited

zhengchenyu commented Jan 12, 2024 •

edited