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

Support use default block size to replace the Hdfs ufs #14324

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

Support use default block size to replace the Hdfs ufs #14324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e1e2060
Support use default block size to replace the HdfsUnderFileSystem blo…
maobaolong Oct 28, 2021
06c39f9
Move the customize block size logic to high level
maobaolong Dec 22, 2021
5584b61
Update core/common/src/main/java/alluxio/conf/PropertyKey.java
maobaolong Jun 12, 2024
5e8583d
Update core/common/src/main/java/alluxio/conf/PropertyKey.java
maobaolong Jun 12, 2024
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: 10 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 @@ -3927,6 +3927,14 @@ public String toString() {
.setDescription("Preferred media type while storing file's blocks.")
.setScope(Scope.CLIENT)
.build();

public static final PropertyKey USER_BLOCK_SIZE_OVERRIDE_UFS_ENABLED =
new Builder(Name.USER_BLOCK_SIZE_ENABLED)
.setDefaultValue(false)
.setDescription("When reading a file from UFS, whether Alluxio should ignore the UFS block size and rely on the Alluxio configuration.")
.setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
.setScope(Scope.ALL)
.build();
public static final PropertyKey USER_BLOCK_SIZE_BYTES_DEFAULT =
new Builder(Name.USER_BLOCK_SIZE_BYTES_DEFAULT)
.setDefaultValue("64MB")
Expand Down Expand Up @@ -6281,6 +6289,8 @@ public static final class Name {
"alluxio.user.block.read.metrics.enabled";
public static final String USER_BLOCK_REMOTE_READ_BUFFER_SIZE_BYTES =
"alluxio.user.block.remote.read.buffer.size.bytes";
public static final String USER_BLOCK_SIZE_ENABLED =
"alluxio.user.block.size.enabled";
public static final String USER_BLOCK_SIZE_BYTES_DEFAULT =
"alluxio.user.block.size.bytes.default";
public static final String USER_BLOCK_READ_RETRY_SLEEP_MIN =
Expand Down
3 changes: 3 additions & 0 deletions core/server/master/src/main/java/alluxio/master/file/InodeSyncStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,9 @@ static void loadFileMetadataInternal(RpcContext rpcContext, LockedInodePath inod
}
ufsLength = ((UfsFileStatus) context.getUfsStatus()).getContentLength();
long blockSize = ((UfsFileStatus) context.getUfsStatus()).getBlockSize();
if (ServerConfiguration.getBoolean(PropertyKey.USER_BLOCK_SIZE_ENABLED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this only reads the server-side default, so it doesn't take your path config or configuration from your client command. This defeats the purpose of having this property.

@beinan @yuzhu Do you know how to make this respect both the client-side config and path config? I'm afraid making this respect user-side config needs this property value to be in message FileSystemCommonPOptions. Getting the path config is easier than modifying RPC, but I forgot how :(

Copy link
Contributor

Choose a reason for hiding this comment

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

In order to pick up the correct path-specific setting, we will need to resolve the config for the path. The path configs are returned from master

alluxio/core/server/master/src/main/java/alluxio/master/meta/DefaultMetaMaster.java

Line 430 in ef6b7ef

PathPropertiesView pathProperties = mPathProperties.snapshot();

to the client

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

Line 433 in ef6b7ef

return new SpecificPathConfiguration(getClientContext().getClusterConf(),

Then the client will resolve the path config. This is how you set sync.interval using alluxio pathConf add /path then how that takes effect.

The annoying thing is, in the sync process the master is actually the client that needs to do the path resolution, and currently there's no API you can directly call. @maobaolong I will take over this part and add one API for you to use. So in your PR here it should be just an easy call.

blockSize = ServerConfiguration.getBytes(PropertyKey.USER_BLOCK_SIZE_BYTES_DEFAULT);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ggezer @yuzhu I have the feeling that the in-Alluxio block size should always be the Alluxio configuration block size, instead of the UFS one. If the user is not happy with the size config then he/she can use Alluxio path config to update that. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 if you have different block sizes for different paths, you still need to update them with path config anyways so no need to create a new property for this.

}
ufsBlockSizeByte = blockSize != UfsFileStatus.UNKNOWN_BLOCK_SIZE
? blockSize : ufs.getBlockSizeByte(ufsUri.toString());

Expand Down
Loading