iframe-proxy

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

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

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

-  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.

roryqi · 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?

roryqi · 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.

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

-  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

roryqi · 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?

roryqi · 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.

roryqi · 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.

roryqi · 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.

roryqi · 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.

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

-  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

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

 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.

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

-  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.

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

-  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.

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

+      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.

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

+  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?

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

+      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.

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

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


Should we put this logic into the class SelectStorageStrategy?

roryqi · 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

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

+    }
+  }
+
+  @Override


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

roryqi

LGTM, thanks @smallzhongfeng , wait for CI

roryqi · 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

[Improvement] Add hdfs path health check to AppBalanceSelectStorageSt…

f87f484

…rategy

smallzhongfeng added 2 commits September 11, 2022 23:12

f

0d0a832

f

74dd36a

roryqi 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

roryqi 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

roryqi reviewed Sep 22, 2022

View reviewed changes

change name

b049a59

f

d42612b

roryqi 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

roryqi reviewed Sep 26, 2022

View reviewed changes

change string to RemoteStorageInfo

f4e46c2

roryqi 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

roryqi reviewed Oct 9, 2022

View reviewed changes

remove sth

82d36c4

roryqi approved these changes Oct 9, 2022

View reviewed changes

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

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

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?

Uh oh!

codecov-commenter commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryqi commented Sep 14, 2022

Uh oh!

roryqi commented Sep 14, 2022

Uh oh!

smallzhongfeng commented Sep 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallzhongfeng Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuston Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smallzhongfeng commented Sep 20, 2022

Uh oh!

roryqi commented Sep 20, 2022

Uh oh!

smallzhongfeng commented Sep 20, 2022

Uh oh!

roryqi commented Sep 21, 2022

Uh oh!

zuston commented Sep 21, 2022

Uh oh!

roryqi commented Sep 21, 2022

Uh oh!

smallzhongfeng commented Sep 21, 2022

Uh oh!

roryqi commented Sep 22, 2022

Uh oh!

smallzhongfeng commented Sep 22, 2022

Uh oh!

smallzhongfeng commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roryqi commented Sep 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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