Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make alluxio client return block location count configurable #18448

Open
wants to merge 4 commits into
base: master-2.x
Choose a base branch
from
Open

Make alluxio client return block location count configurable #18448

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
25294cb
make alluxio client return block location count configurable
codings-dan Nov 28, 2023
940bbe6
address review comment
codings-dan Dec 13, 2023
a39c6c0
address review comment
codings-dan Dec 22, 2023
a672e64
fix findbugs
codings-dan Dec 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions core/client/fs/src/main/java/alluxio/client/file/BaseFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

locations.addAll(getHostWorkerMap().values());
Collections.shuffle(locations);
List<WorkerNetAddress> addresses = new ArrayList<>(getHostWorkerMap().values());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is copy No.1

Collections.shuffle(addresses);

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is copy No.2

Can you make sure there's only 1 copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
}
blockLocations.add(new BlockLocationInfo(fileBlockInfo, locations));
Expand Down
11 changes: 11 additions & 0 deletions core/common/src/main/java/alluxio/conf/PropertyKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6843,6 +6843,15 @@ public String toString() {
.setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
.setScope(Scope.CLIENT)
.build();
public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_COUNT =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_COUNT =
public static final PropertyKey USER_UFS_BLOCK_LOCATION_RETURN_LIMIT =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

intBuilder(Name.USER_UFS_BLOCK_LOCATION_RETURN_COUNT)
.setDefaultValue(-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.setDescription("The return count of workers as block location if ufs block locations "
+ "are not co-located with any Alluxio workers or is empty. This item should be "
+ "greater than or equal to -1 and '-1' means return all workers")
.setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
.setScope(Scope.CLIENT)
.build();
public static final PropertyKey USER_UFS_BLOCK_READ_LOCATION_POLICY =
classBuilder(Name.USER_UFS_BLOCK_READ_LOCATION_POLICY)
.setDefaultValue("alluxio.client.block.policy.LocalFirstPolicy")
Expand Down Expand Up @@ -9082,6 +9091,8 @@ public static final class Name {
public static final String USER_RPC_RETRY_MAX_SLEEP_MS = "alluxio.user.rpc.retry.max.sleep";
public static final String USER_UFS_BLOCK_LOCATION_ALL_FALLBACK_ENABLED =
"alluxio.user.ufs.block.location.all.fallback.enabled";
public static final String USER_UFS_BLOCK_LOCATION_RETURN_COUNT =
"alluxio.user.block.location.return.count";
public static final String USER_UFS_BLOCK_READ_LOCATION_POLICY =
"alluxio.user.ufs.block.read.location.policy";
public static final String USER_UFS_BLOCK_READ_LOCATION_POLICY_DETERMINISTIC_HASH_SHARDS =
Expand Down