[ISSUE-285][Improvement] Only use HDFS and LOCALFILE storageType in the test #360

tiantingting5435 · 2022-11-26T20:20:15Z

What changes were proposed in this pull request?

We add some config options:

rss.test.mode.enable, for rss server;
mapreduce.rss.test.mode.enable, for mr client;
spark.rss.test.mode.enable, for spark client.

When we use HDFS or LOCALFILE storageType in the client or shuffle server, we should throw an exception if run with test mode.

Why are the changes needed?

HDFS and LOCALFILE storageType have poor performance, but they are useful for tests. We don't recommend to use them.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Test locally

jerqi · 2022-11-27T02:28:18Z

We should also check client's storageType

incubator-uniffle/client-spark/spark2/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

Line 249 in 57a834b

String storageType = sparkConf.get(RssSparkConfig.RSS_STORAGE_TYPE.key());

incubator-uniffle/client-spark/spark3/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

Line 305 in 57a834b

String storageType = sparkConf.get(RssSparkConfig.RSS_STORAGE_TYPE.key());

incubator-uniffle/client-mr/src/main/java/org/apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java

Line 163 in 57a834b

    
           String storageType = RssMRUtils.getString(extraConf, conf, RssMRConfig.RSS_STORAGE_TYPE);

jerqi · 2022-11-27T02:31:08Z

You should fix the code style issue

tiantingting5435 · 2022-11-27T12:52:26Z

@jerqi Hi, would you mind to take another look?

jerqi · 2022-11-27T15:00:41Z

Please fix code style and ut. You can use the command mvn package -Pspark2, mvn package -Pspark3, mvn package -Pspark3 to run the uts on your local machine first.

tiantingting5435 · 2022-11-27T19:13:53Z

Please fix code style and ut. You can use the command mvn package -Pspark2, mvn package -Pspark3, mvn package -Pspark3 to run the uts on your local machine first.

@jerqi Thanks for the heads up, I think I've finished fixing the current known bugs.

jerqi · 2022-11-28T02:11:46Z

You need to resolve the conflicts with master. you can use the command

 git remote add upstream git@github.com:apache/incubator-uniffle.git
 git fetch upstream
 git merge upstream/master

solve the conflicts and then

git add xxxxx
git merge --continue

codecov-commenter · 2022-11-28T02:32:52Z

Codecov Report

Merging #360 (f868b54) into master (c503f8f) will decrease coverage by 0.72%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##             master     #360      +/-   ##
============================================
- Coverage     58.59%   57.86%   -0.73%     
+ Complexity     1588     1371     -217     
============================================
  Files           193      171      -22     
  Lines         10888     9067    -1821     
  Branches        954      797     -157     
============================================
- Hits           6380     5247    -1133     
+ Misses         4132     3486     -646     
+ Partials        376      334      -42

Impacted Files	Coverage Δ
...va/org/apache/uniffle/client/util/ClientUtils.java	`52.27% <0.00%> (-5.23%)`	⬇️
...rg/apache/uniffle/client/util/RssClientConfig.java	`0.00% <ø> (ø)`
.../java/org/apache/uniffle/server/ShuffleServer.java	`67.39% <40.00%> (-1.03%)`	⬇️
.../org/apache/uniffle/common/config/RssBaseConf.java	`93.63% <100.00%> (+0.20%)`	⬆️
.../java/org/apache/hadoop/mapreduce/RssMRConfig.java
...apache/hadoop/mapreduce/v2/app/RssMRAppMaster.java
.../java/org/apache/spark/shuffle/RssSparkConfig.java
...org/apache/spark/shuffle/RssSparkShuffleUtils.java
.../hadoop/mapreduce/task/reduce/RssEventFetcher.java
...rg/apache/hadoop/mapred/RssMapOutputCollector.java
... and 16 more

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

jerqi · 2022-11-28T03:08:29Z

client-spark/spark2/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

@@ -283,7 +283,7 @@ public <K, V, C> ShuffleHandle registerShuffle(int shuffleId, int numMaps, Shuff

  private void startHeartbeat() {
    shuffleWriteClient.registerApplicationInfo(appId, heartbeatTimeout, user);
-    if (!sparkConf.getBoolean(RssSparkConfig.RSS_TEST_FLAG.key(), false) && !heartbeatStarted) {
+    if (!sparkConf.getBoolean(RssSparkConfig.RSS_TEST_MODE.key(), false) && !heartbeatStarted) {


jerqi · 2022-11-28T03:09:10Z

client-spark/spark2/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java

@@ -188,7 +188,7 @@ public RssShuffleManager(SparkConf sparkConf, boolean isDriver) {
    // External shuffle service is not supported when using remote shuffle service
    sparkConf.set("spark.shuffle.service.enabled", "false");
    LOG.info("Disable external shuffle service in RssShuffleManager.");
-    if (!sparkConf.getBoolean(RssSparkConfig.RSS_TEST_FLAG.key(), false)) {
+    if (!sparkConf.getBoolean(RssSparkConfig.RSS_TEST_MODE.key(), false)) {


We shouldn't modify this place. It will cause that our spark2 integration tests fail.

The change here is to unify the test flags and use rss.test.mode.enable to confirm if it is a test mode. Using multiple parameters to mark a test mode tends to create redundancy and confusion in the parameters, UT fails I can fix it.

It's a little difficult to unify them. Because RSS_TEST_FLAG is used for ShuffleFlushManagerTest, RSS_TEST_MODE is used for integration tests and uts.

They have different effect though they have similar names. For integraion tests, we need to set RSS_TEST_FLAG false value and set RSS_TEST_MODE true value.

I see what you mean, give me some time to restore the original design.

…he test

jerqi

LGTM, thanks @tiantingting5435 a lot. It's great work!

jerqi linked an issue Nov 27, 2022 that may be closed by this pull request

[Improvement] Only Use HDFS and LOCALFILE storageType in the test #285

Closed

3 tasks

tiantingting5435 force-pushed the issue-285 branch 2 times, most recently from cc07b01 to b78db09 Compare November 27, 2022 11:57

jerqi changed the title ~~[ISSUE-285][Improvement] Only Use HDFS and LOCALFILE storageType in the test~~ [ISSUE-285][Improvement] Only use HDFS and LOCALFILE storageType in the test Nov 27, 2022

tiantingting5435 force-pushed the issue-285 branch from fa2d452 to 005245d Compare November 27, 2022 18:54

jerqi reviewed Nov 28, 2022

View reviewed changes

tiantingting5435 added 7 commits November 29, 2022 22:19

[ISSUE-285][Improvement] Only use HDFS and LOCALFILE storageType in t…

1a10115

…he test

fix checkstyle and syntax

eb06965

add storageType check in mr and spark client

c7d4bf6

Use the same style of rss.test.mode naming and code optimisation

3d1231c

fix ut

c9bb6e2

fix checkStyle

13b3d16

fallback about conf RSS_TEST_FLAG

f868b54

tiantingting5435 force-pushed the issue-285 branch from 2b347f8 to f868b54 Compare November 29, 2022 14:30

jerqi approved these changes Nov 29, 2022

View reviewed changes

jerqi merged commit 884921b into apache:master Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE-285][Improvement] Only use HDFS and LOCALFILE storageType in the test #360

[ISSUE-285][Improvement] Only use HDFS and LOCALFILE storageType in the test #360

tiantingting5435 commented Nov 26, 2022 •

edited

jerqi commented Nov 27, 2022

jerqi commented Nov 27, 2022

tiantingting5435 commented Nov 27, 2022

jerqi commented Nov 27, 2022

tiantingting5435 commented Nov 27, 2022

jerqi commented Nov 28, 2022 •

edited

codecov-commenter commented Nov 28, 2022 •

edited

jerqi Nov 28, 2022

jerqi Nov 28, 2022

tiantingting5435 Nov 28, 2022

jerqi Nov 28, 2022

jerqi Nov 28, 2022

tiantingting5435 Nov 28, 2022

jerqi left a comment

[ISSUE-285][Improvement] Only use HDFS and LOCALFILE storageType in the test #360

[ISSUE-285][Improvement] Only use HDFS and LOCALFILE storageType in the test #360

Conversation

tiantingting5435 commented Nov 26, 2022 • edited

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?

jerqi commented Nov 27, 2022

jerqi commented Nov 27, 2022

tiantingting5435 commented Nov 27, 2022

jerqi commented Nov 27, 2022

tiantingting5435 commented Nov 27, 2022

jerqi commented Nov 28, 2022 • edited

codecov-commenter commented Nov 28, 2022 • edited

Codecov Report

jerqi Nov 28, 2022

Choose a reason for hiding this comment

jerqi Nov 28, 2022

Choose a reason for hiding this comment

tiantingting5435 Nov 28, 2022

Choose a reason for hiding this comment

jerqi Nov 28, 2022

Choose a reason for hiding this comment

jerqi Nov 28, 2022

Choose a reason for hiding this comment

tiantingting5435 Nov 28, 2022

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

tiantingting5435 commented Nov 26, 2022 •

edited

jerqi commented Nov 28, 2022 •

edited

codecov-commenter commented Nov 28, 2022 •

edited