Make alluxio client return block location count configurable #18448

codings-dan · 2023-11-28T07:14:09Z

What changes are proposed in this pull request?

make alluxio client return block location count configurable

Why are the changes needed?

When accessing alluxio in the hive + yarn + mr scenario, the metadata will be accessed and split before the job is submitted, which will generate the job.splitmetainfo file. This file should not be too large. When accessing alluxio, if the data does not exist in alluxio, all worker address information will be returned, which will expand the size of the generated job.splitmetainfo file and eventually result in an error as below. This PR makes the number of returned worker addresses configurable

org.apache.hadoop.yarn.exceptions.YarnRuntimeException: java.io.IOException: Split metadata size exceeded 50000000. Aborting job job_1698927225178_1330717

Does this PR introduce any user facing changes?

No

codings-dan · 2023-11-28T07:15:11Z

@maobaolong @jiacheliu3 @dbw9580 Could you help take a look at the pull request, thanks in advance!

jiacheliu3

Mostly LGTM with some comments, thanks for the improvement

jiacheliu3 · 2023-11-30T15:16:10Z

core/common/src/main/java/alluxio/conf/PropertyKey.java

@@ -6843,6 +6843,15 @@ public String toString() {
          .setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
          .setScope(Scope.CLIENT)
          .build();
+  public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_COUNT =
+      intBuilder(Name.USER_UFS_BLOCK_LOCATION_RETURN_COUNT)
+          .setDefaultValue(-1)


you can just set default to Integer.MAX_VALUE and that means no limit

jiacheliu3 · 2023-11-30T15:17:18Z

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

@@ -288,8 +288,14 @@ public List<BlockLocationInfo> getBlockLocations(URIStatus status)
        if (locations.isEmpty() && mFsContext.getPathConf(new AlluxioURI(status.getPath()))
            .getBoolean(PropertyKey.USER_UFS_BLOCK_LOCATION_ALL_FALLBACK_ENABLED)) {
          // Case 2: Fallback to add all workers to locations so some apps (Impala) won't panic.
-          locations.addAll(getHostWorkerMap().values());
-          Collections.shuffle(locations);
+          List<WorkerNetAddress> addresses = new ArrayList<>(getHostWorkerMap().values());


this is copy No.1

jiacheliu3 · 2023-11-30T15:18:05Z

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

+          int count = mFsContext.getClusterConf().getInt(
+              PropertyKey.USER_UFS_BLOCK_LOCATION_RETURN_COUNT);
+          count = count >= 0 ? count : Integer.MAX_VALUE;
+          addresses = addresses.subList(0, Math.min(addresses.size(), count));
+          locations.addAll(addresses);


and this is copy No.2

Can you make sure there's only 1 copy?

jiacheliu3 · 2023-11-30T15:18:20Z

core/common/src/main/java/alluxio/conf/PropertyKey.java

@@ -6843,6 +6843,15 @@ public String toString() {
          .setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
          .setScope(Scope.CLIENT)
          .build();
+  public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_COUNT =


Suggested change

public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_COUNT =

public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_LIMIT =

alluxio-bot · 2023-12-13T11:11:49Z

Automated checks report:

PR title follows the conventions: PASS
Commits associated with Github account: FAIL
- It looks like your commits can't be linked to a valid Github account.
  Your commits are made with the email [email protected], which does not allow your contribution to be tracked by Github.
  See this link for possible reasons this might be happening.
  To change the author email address that your most recent commit was made under, you can run:
```
git -c user.name="Name" -c user.email="Email" commit --amend --reset-author
```
  See this answer for more details about how to change commit email addresses.
  Once the author email address has been updated, update the pull request by running:
```
git push --force https://github.com/codings-dan/alluxio.git alluxio-community-client-block-loc-configurable
```

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

codings-dan · 2023-12-13T11:13:10Z

@jiacheliu3 Thanks a lot for reviewing the pull request! PTAL

jiacheliu3

Thanks for the work!

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

jiacheliu3 · 2023-12-22T03:32:25Z

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

@@ -288,15 +288,27 @@ public List<BlockLocationInfo> getBlockLocations(URIStatus status)
        if (locations.isEmpty() && mFsContext.getPathConf(new AlluxioURI(status.getPath()))
            .getBoolean(PropertyKey.USER_UFS_BLOCK_LOCATION_ALL_FALLBACK_ENABLED)) {
          // Case 2: Fallback to add all workers to locations so some apps (Impala) won't panic.
-          locations.addAll(getHostWorkerMap().values());
-          Collections.shuffle(locations);
+          PropertyKey locKey = PropertyKey.USER_UFS_BLOCK_LOCATION_RETURN_LIMIT;


Suggested change

PropertyKey locKey = PropertyKey.USER_UFS_BLOCK_LOCATION_RETURN_LIMIT;

PropertyKey locKey = PropertyKey.USER_UFS_BLOCK_LOCATION_FALLBACK_RETURN_LIMIT;

let's rename this property key name so it better reflects what it does in the code

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

jiacheliu3 · 2023-12-22T03:34:30Z

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

+                + " should not be set to a negative number");
+          }
+          List<WorkerNetAddress> addresses = getShuffleWorkerAddressList();
+          locations.addAll(addresses.subList(0, Math.min(addresses.size(), count)));


this line has many copies, you can just write an easy for loop

for (i = 0; i < count; i++) { locations.add(addresses.get(i)); }

jiacheliu3 · 2023-12-22T03:34:48Z

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

-          locations.addAll(getHostWorkerMap().values());
-          Collections.shuffle(locations);
+          PropertyKey locKey = PropertyKey.USER_UFS_BLOCK_LOCATION_RETURN_LIMIT;
+          int count = mFsContext.getClusterConf().getInt(locKey);


use path conf

jiacheliu3 · 2023-12-22T03:36:17Z

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

+    List<BlockWorkerInfo> workers = mFsContext.getCachedWorkers();
+    Collections.shuffle(workers);
+    return workers.stream().map(BlockWorkerInfo::getNetAddress).collect(toList());


why not keep using getHostWorkerMap()? This new code may not keep the same behavior.

will remove the method and use getHostWorkerMap()

qian0817 · 2024-01-02T07:21:41Z

core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java

@@ -288,8 +288,21 @@ public List<BlockLocationInfo> getBlockLocations(URIStatus status)
        if (locations.isEmpty() && mFsContext.getPathConf(new AlluxioURI(status.getPath()))
            .getBoolean(PropertyKey.USER_UFS_BLOCK_LOCATION_ALL_FALLBACK_ENABLED)) {
          // Case 2: Fallback to add all workers to locations so some apps (Impala) won't panic.


If this situation occurs, can you directly return the default value defined here? This is more friendly to Impala's file handle cache and data cache. This is because Impala will use consistent hash to schedule data scan fragment, and random block location returns will reduce the cache hit rate.

Thanks for the review. I don't quite understand the change method you are talking about, or is this what you are talking about?

make alluxio client return block location count configurable

25294cb

jiacheliu3 requested changes Nov 30, 2023

View reviewed changes

codings-dan force-pushed the alluxio-community-client-block-loc-configurable branch 2 times, most recently from 685323e to 6a9e7c0 Compare December 13, 2023 11:17

address review comment

940bbe6

codings-dan force-pushed the alluxio-community-client-block-loc-configurable branch from 6a9e7c0 to 940bbe6 Compare December 13, 2023 11:20

jiacheliu3 requested changes Dec 22, 2023

View reviewed changes

codings-dan added 2 commits December 22, 2023 15:14

address review comment

a39c6c0

fix findbugs

a672e64

qian0817 suggested changes Jan 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make alluxio client return block location count configurable #18448

Make alluxio client return block location count configurable #18448

codings-dan commented Nov 28, 2023

codings-dan commented Nov 28, 2023

jiacheliu3 left a comment

jiacheliu3 Nov 30, 2023

jiacheliu3 Nov 30, 2023

jiacheliu3 Nov 30, 2023

codings-dan Dec 13, 2023

jiacheliu3 Nov 30, 2023

codings-dan Dec 13, 2023

alluxio-bot commented Dec 13, 2023

codings-dan commented Dec 13, 2023

jiacheliu3 left a comment

jiacheliu3 Dec 22, 2023

jiacheliu3 Dec 22, 2023

codings-dan Dec 22, 2023

jiacheliu3 Dec 22, 2023

codings-dan Dec 22, 2023

jiacheliu3 Dec 22, 2023

codings-dan Dec 22, 2023

jiacheliu3 Dec 22, 2023

codings-dan Dec 22, 2023

qian0817 Jan 2, 2024

codings-dan Jan 19, 2024

	public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_COUNT =
	public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_LIMIT =

	PropertyKey locKey = PropertyKey.USER_UFS_BLOCK_LOCATION_RETURN_LIMIT;
	PropertyKey locKey = PropertyKey.USER_UFS_BLOCK_LOCATION_FALLBACK_RETURN_LIMIT;

Make alluxio client return block location count configurable #18448

Are you sure you want to change the base?

Make alluxio client return block location count configurable #18448

Conversation

codings-dan commented Nov 28, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

codings-dan commented Nov 28, 2023

jiacheliu3 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

alluxio-bot commented Dec 13, 2023

codings-dan commented Dec 13, 2023

jiacheliu3 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment