[Improvement] Add hdfs path health check to AppBalanceSelectStorageStrategy #210

smallzhongfeng · 2022-09-11T14:19:58Z

What changes were proposed in this pull request?

The detection logic of abnormal paths is also added to the APP_BALANCE strategy.

Why are the changes needed?

Anomaly detection can be performed when selecting a remote path to avoid selecting a problematic path.

Does this PR introduce any user-facing change?

The parameters of the original IO_SAMPLE strategy are reused and the names are changed.

rss.coordinator.remote.storage.select.strategy support APP_BALANCE and IO_SAMPLE, APP_BALANCE selection strategy based on the number of apps, IO_SAMPLE selection strategy based on time consumption of reading and writing files.
rss.coordinator.remote.storage.schedule.time , if user choose IO_SAMPLE, file will be read and written at regular intervals.
rss.coordinator.remote.storage.schedule.file.size , the size of each read / write HDFS file.
rss.coordinator.remote.storage.schedule.access.times, number of times to read and write HDFS files.

How was this patch tested?

Passed uts.

…rategy

codecov-commenter · 2022-09-11T14:38:09Z

Codecov Report

Merging #210 (82d36c4) into master (f1cb43f) will decrease coverage by 0.15%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master     #210      +/-   ##
============================================
- Coverage     59.31%   59.16%   -0.16%     
+ Complexity     1345     1339       -6     
============================================
  Files           161      163       +2     
  Lines          8780     8810      +30     
  Branches        828      833       +5     
============================================
+ Hits           5208     5212       +4     
- Misses         3304     3332      +28     
+ Partials        268      266       -2

Impacted Files	Coverage Δ
...fle/coordinator/AbstractSelectStorageStrategy.java	`20.00% <20.00%> (ø)`
...nator/LowestIOSampleCostSelectStorageStrategy.java	`71.42% <69.35%> (-2.65%)`	⬇️
...apache/uniffle/coordinator/ApplicationManager.java	`83.80% <70.73%> (-6.47%)`	⬇️
...e/coordinator/AppBalanceSelectStorageStrategy.java	`76.00% <75.00%> (+3.65%)`	⬆️
...a/org/apache/uniffle/common/RemoteStorageInfo.java	`96.29% <100.00%> (+1.85%)`	⬆️
...rg/apache/uniffle/coordinator/CoordinatorConf.java	`97.05% <100.00%> (ø)`
...pache/uniffle/server/ShuffleServerGrpcService.java	`0.90% <0.00%> (-0.02%)`	⬇️
...orage/handler/impl/LocalFileServerReadHandler.java	`77.96% <0.00%> (ø)`
... and 2 more

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

jerqi · 2022-09-13T02:21:29Z

coordinator/src/main/java/org/apache/uniffle/coordinator/SelectStorageStrategy.java


-  Map<String, RankValue> getRemoteStoragePathRankValue();
+  void setFs(FileSystem fs);


We would better not assume our remote storage is only FileSystem.

Could you elaborate more, or give me some other examples ?

If we want to support S3 , COS, Redis or some DBs, it's not suitable for us.

Maybe I can remove this method, thinking that if I want to support redis or other databases, I need to re implement the sortPathByRankValue method. It's a huge job.

jerqi · 2022-09-14T10:47:59Z

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?

jerqi · 2022-09-14T10:48:58Z

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?

smallzhongfeng · 2022-09-14T16:26:40Z

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?

No problem, I got it.

jerqi · 2022-09-15T15:31:26Z

coordinator/src/main/java/org/apache/uniffle/coordinator/SelectStorageStrategy.java

-  Map<String, RemoteStorageInfo> getAppIdToRemoteStorageInfo();
-
-  Map<String, RankValue> getRemoteStoragePathRankValue();
+  List<Map.Entry<String, RankValue>> sortPathByRankValue(String path, Path testPath, long time, boolean isHealthy);


Path seems to be a concept of filesystem. It may be not general.

Maybe we can support fileSystem at the beginning. If we need to support db, then there may be more things to consider, or we can add a parameter. If it is a file supported by fileSystem, we will turn on this switch.

For S3 , COS, it's OK, right?

At least We need consider the object Storage.

�IIRC, object storage such as s3 also has the concept of Path.

But cos doesn't seem to have this concept.

As I know, the object storage also can be accessed by Hadoop Filesystem Interface (HCFS), like s3/oss/cos. So this is not a problem

But if we want to involve more storages which are not implemented by HCFS or speed up specified api which is only compatible with original storage api, maybe we should implement uniffle's own filesystem, it can refer to Flink.

Please let me know if I'm wrong.

Thank you very much for your reminder. Although I am not very impressed with this, I have used a similar method to read files in s3.

smallzhongfeng · 2022-09-20T11:26:27Z

For object storage, there may be more S3, but for cos, I don't know this very well. Do you have any good suggestions? @jerqi

jerqi · 2022-09-20T12:25:58Z

For object storage, there may be more S3, but for cos, I don't know this very well. Do you have any good suggestions? @jerqi

Do the S3 have the concept of path? It only have the concept of bucket.

smallzhongfeng · 2022-09-20T17:01:20Z

Could we support anomaly detection of hdfs files first?

jerqi · 2022-09-21T02:15:47Z

Could we support anomaly detection of hdfs files first?

We should have more general interface. We can only implement part of them.

zuston · 2022-09-21T06:50:29Z

For object storage, there may be more S3, but for cos, I don't know this very well. Do you have any good suggestions? @jerqi

Do the S3 have the concept of path? It only have the concept of bucket.

Yes, the s3 compatible filesystem of Hadoop has implemented the abstraction of Path, but it's slower than the s3 original api when using list files/dirs. I think cos is the same with s3.

By the way, I implemented the hadoop filesystem api for internal object store in the past, so have some experience on it.

jerqi · 2022-09-21T10:04:31Z

For object storage, there may be more S3, but for cos, I don't know this very well. Do you have any good suggestions? @jerqi

Do the S3 have the concept of path? It only have the concept of bucket.

Yes, the s3 compatible filesystem of Hadoop has implemented the abstraction of Path, but it's slower than the s3 original api when using list files/dirs. I think cos is the same with s3.

By the way, I implemented the hadoop filesystem api for internal object store in the past, so have some experience on it.

We shouldn't relay on the filesystem of object storage too much. Some operations of object storage is slow. For example, list operation, rename operation and append operation.

smallzhongfeng · 2022-09-21T18:32:14Z

We should have more general interface. We can only implement part of them.

I think we only need to provide a path that includes the remote, and the rest of the methods can be implemented by judging different storage systems.

jerqi · 2022-09-22T02:25:44Z

We should have more general interface. We can only implement part of them.

I think we only need to provide a path that includes the remote, and the rest of the methods can be implemented by judging different storage systems.

My doubt point is that the concept of path is not general one for the storage.

smallzhongfeng · 2022-09-22T02:51:11Z

So I did not use the concept of path for the time being, but used a string to represent.

smallzhongfeng · 2022-09-22T02:52:22Z

So I did not use the concept of path for the time being, but used a string to represent.

The concept of subsequent use of buckets or paths can be implemented through the readAndWrite interface.

jerqi · 2022-09-22T02:54:30Z

So I did not use the concept of path for the time being, but used a string to represent.

The concept of subsequent use of buckets or paths can be implemented through the readAndWrite interface.

It's ok.

jerqi · 2022-09-22T03:01:50Z

coordinator/src/main/java/org/apache/uniffle/coordinator/SelectStorageStrategy.java

-  Map<String, RemoteStorageInfo> getAppIdToRemoteStorageInfo();
-
-  Map<String, RankValue> getRemoteStoragePathRankValue();
+  List<Map.Entry<String, RankValue>> readAndWrite(String path);


I have a little doubt, why do the readAndWrite become the only interface method for SelectStorageStrategy? I can rank by many methods, it's not necessary to read and write.

The meaning of the readAndWrite interface is to perform anomaly detection of remote paths. For hdfs, reading and writing files is a more direct way to detect, so I took this name, but the real comparison method comes from sortPathByRankValue, which I do not have. The reason for defining it as an interface is because the objects to be sorted may be different, and this is left to different storage methods for implementation.

readAndWrite is not a good name. It's strange for the interface SelectStorageStrategy.

OK, updated.

Should we add a method pickStorage or selectStorage for this interface?

Yes, that might make more sense for this interface, I will add.

smallzhongfeng · 2022-09-23T13:15:46Z

PTAL @jerqi

jerqi · 2022-09-24T06:39:37Z

coordinator/src/main/java/org/apache/uniffle/coordinator/SelectStorageStrategy.java

 import org.apache.uniffle.coordinator.LowestIOSampleCostSelectStorageStrategy.RankValue;

 public interface SelectStorageStrategy {

-  RemoteStorageInfo pickRemoteStorage(String appId);
+  List<Map.Entry<String, RankValue>> detectStorage(String path);


path -> uri?
Path is not general concept for storage. Maybe we should use uri.

OK, good suggestion.

jerqi · 2022-09-24T06:40:15Z

coordinator/src/main/java/org/apache/uniffle/coordinator/SelectStorageStrategy.java

-  Map<String, RemoteStorageInfo> getAppIdToRemoteStorageInfo();
-
-  Map<String, RankValue> getRemoteStoragePathRankValue();
+  List<Map.Entry<String, RankValue>> pickStorage(List<Map.Entry<String, RankValue>> pathList);


Could we give pathList a better name?

Do you have any good advice?

What about uriRankings?

uriList or uris may be better?
Why do the method pickStorage return a list?

I modified it to let pickStorage return to a selected path.

jerqi · 2022-09-26T06:10:51Z

coordinator/src/main/java/org/apache/uniffle/coordinator/SelectStorageStrategy.java

-  Map<String, RemoteStorageInfo> getAppIdToRemoteStorageInfo();
-
-  Map<String, RankValue> getRemoteStoragePathRankValue();
+  String pickStorage(List<Map.Entry<String, RankValue>> uris, String appId);


Should we return RemoteStorageInfo?

No need, because the return value of pickStorage only needs to be passed to incRemoteStorageCounter, incRemoteStorageCounter only needs String type, and String type is more general than RemoteStorageInfo.

No need, because the return value of pickStorage only needs to be passed to incRemoteStorageCounter, incRemoteStorageCounter only needs String type, and String type is more general than RemoteStorageInfo.

String is too general to describe what we want to return. RemoteStorageInfo is more accurate.

OK, updated.

jerqi · 2022-09-27T02:12:59Z

coordinator/src/main/java/org/apache/uniffle/coordinator/AppBalanceSelectStorageStrategy.java

+      Path testPath = new Path(rssTest);
+      try {
+        FileSystem fs = HadoopFilesystemProvider.getFilesystem(remotePath, hdfsConf);
+        for (int j = 0; j < readAndWriteTimes; j++) {


If we just detect the health of storage, it seems unnecessary for us write and read them multiple times.

For appBalance, it can indeed be changed once, but for io sampling, the number of times can be increased, perhaps we can set the parameter to 1 by default.

For appBalance, it shouldn't be a configuration option.

jerqi · 2022-09-27T02:16:22Z

coordinator/src/main/java/org/apache/uniffle/coordinator/AppBalanceSelectStorageStrategy.java

+  private final Configuration hdfsConf;
+  private final int fileSize;
+  private final int readAndWriteTimes;
+  private boolean remotePathIsHealthy = true;


What does this variable remotePathIsHealthy mean?
This class is a strategy. I can't get the meaning of this varibale.

This variable is used to determine whether the path is normal, and to pass the parameter into the method sortPathByRankValue.

This variable is used to determine whether the path is normal, and to pass the parameter into the method sortPathByRankValue.

One strategy should have multiple paths, every path is healthy, the variable is true?

I know that there are many paths, but this attribute is applied to each path. The healthy path is processed by sortPathByRankValue according to remotePathIsHealthy as true, and corresponding logic is processed when it is false.

You can look at the logic of sortPathByRankValue. When we judge by this parameter, it is a normal path, and then deal with it accordingly.

Is the variable necessary to become member field?

jerqi · 2022-09-27T02:19:32Z

coordinator/src/main/java/org/apache/uniffle/coordinator/AppBalanceSelectStorageStrategy.java

+      uris = uris.stream().filter(rv -> rv.getValue().getReadAndWriteTime().get() != Long.MAX_VALUE).sorted(
+          Comparator.comparingInt(entry -> entry.getValue().getAppNum().get())).collect(Collectors.toList());
+    } else {
+      // If all paths are unhealthy, assign paths according to the number of apps


Should we add some logs and metrics when all paths are unhealthy?

OK, I will add.

jerqi · 2022-09-27T02:22:09Z

coordinator/src/main/java/org/apache/uniffle/coordinator/ApplicationManager.java

+        conf.getLong(CoordinatorConf.COORDINATOR_REMOTE_STORAGE_SCHEDULE_TIME), TimeUnit.MILLISECONDS);
+  }
+
+  public void checkReadAndWrite() {


Should we put this logic into the class SelectStorageStrategy?

jerqi · 2022-09-27T08:47:44Z

Maybe we should have a abstract class to define the common behaviors of SelectStorageStrategy.

smallzhongfeng · 2022-10-08T13:14:59Z

This modification has done three things

An abstract class AbstractSelectStorageStrategy is added to provide a method for reading and writing hdfs paths.
Added logs of the sorted storage list.
Removed a global variable of isHealthy and changed it to a local variable.

smallzhongfeng · 2022-10-09T02:54:58Z

PTAL @jerqi

jerqi · 2022-10-09T03:07:06Z

coordinator/src/main/java/org/apache/uniffle/coordinator/AbstractSelectStorageStrategy.java

+    }
+  }
+
+  @Override


If we don't implement the method detectStorage and pickStorage, we can remove them in the abstract class.

jerqi

LGTM, thanks @smallzhongfeng , wait for CI

jerqi · 2022-10-09T08:46:28Z

https://github.com/apache/incubator-uniffle/actions/runs/3213433147 There is a flay test. Could you help me fix it?
test-reports-spark3.2.0 (1).zip

smallzhongfeng · 2022-10-09T12:01:58Z

https://github.com/apache/incubator-uniffle/actions/runs/3213433147 There is a flay test. Could you help me fix it? test-reports-spark3.2.0 (1).zip

OK, I will fix it.

[Improvement] Add hdfs path health check to AppBalanceSelectStorageSt…

f87f484

…rategy

smallzhongfeng added 2 commits September 11, 2022 23:12

f

0d0a832

f

74dd36a

jerqi reviewed Sep 13, 2022

View reviewed changes

fix conflict

3c095b3

smallzhongfeng force-pushed the add-unhealthyPathCheck branch from 6f1cf29 to 3c095b3 Compare September 15, 2022 12:13

smallzhongfeng and others added 3 commits September 15, 2022 20:16

Merge branch 'master' into add-unhealthyPathCheck

175417e

remove setFs

ed2d12c

f

c80d894

jerqi reviewed Sep 15, 2022

View reviewed changes

add hdfs limit

cf974ec

smallzhongfeng added 2 commits September 22, 2022 02:25

change interface

ce0635a

f

f264f37

jerqi reviewed Sep 22, 2022

View reviewed changes

change name

b049a59

f

d42612b

jerqi reviewed Sep 24, 2022

View reviewed changes

smallzhongfeng added 2 commits September 24, 2022 23:37

only return suitable storage

a304fd5

f

2a8cbcb

smallzhongfeng closed this Sep 24, 2022

smallzhongfeng reopened this Sep 24, 2022

jerqi reviewed Sep 26, 2022

View reviewed changes

change string to RemoteStorageInfo

f4e46c2

jerqi reviewed Sep 27, 2022

View reviewed changes

Merge branch 'master' into add-unhealthyPathCheck

bc76389

smallzhongfeng and others added 6 commits September 29, 2022 00:16

Update LowestIOSampleCostSelectStorageStrategy.java

d16396f

Update LowestIOSampleCostSelectStorageStrategy.java

f29074b

Update LowestIOSampleCostSelectStorageStrategy.java

282c949

remove isHealthy and add interface checkStorages

0173b1d

add abstract class

38cbc1c

f

d25660f

smallzhongfeng closed this Oct 8, 2022

smallzhongfeng reopened this Oct 8, 2022

This comment was marked as resolved.

Sign in to view

jerqi reviewed Oct 9, 2022

View reviewed changes

remove sth

82d36c4

jerqi approved these changes Oct 9, 2022

View reviewed changes

jerqi merged commit c89f95c into apache:master Oct 9, 2022


		Map<String, RankValue> getRemoteStoragePathRankValue();
		void setFs(FileSystem fs);

[Improvement] Add hdfs path health check to AppBalanceSelectStorageStrategy #210

[Improvement] Add hdfs path health check to AppBalanceSelectStorageStrategy #210

Conversation

smallzhongfeng commented Sep 11, 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 11, 2022 • 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

jerqi commented Sep 14, 2022

jerqi commented Sep 14, 2022

smallzhongfeng commented Sep 14, 2022

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

smallzhongfeng Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuston Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smallzhongfeng commented Sep 20, 2022

jerqi commented Sep 20, 2022

smallzhongfeng commented Sep 20, 2022

jerqi commented Sep 21, 2022

zuston commented Sep 21, 2022

jerqi commented Sep 21, 2022

smallzhongfeng commented Sep 21, 2022

jerqi commented Sep 22, 2022

smallzhongfeng commented Sep 22, 2022

smallzhongfeng commented Sep 22, 2022 • edited Loading

jerqi commented Sep 22, 2022

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

smallzhongfeng Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

smallzhongfeng commented Sep 23, 2022

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

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

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 Sep 27, 2022

smallzhongfeng commented Oct 8, 2022 • edited Loading

This comment was marked as resolved.

smallzhongfeng commented Oct 9, 2022

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

codecov-commenter commented Sep 11, 2022 •

edited

Loading

smallzhongfeng Sep 16, 2022 •

edited

Loading

zuston Sep 21, 2022 •

edited

Loading

smallzhongfeng commented Sep 22, 2022 •

edited

Loading

smallzhongfeng Sep 23, 2022 •

edited

Loading

smallzhongfeng commented Oct 8, 2022 •

edited

Loading