Skip to content

feat: add more options on mount#172

Merged
bigbigxu merged 1 commit intoCurvineIO:mainfrom
zuston:mountOptions
Aug 12, 2025
Merged

feat: add more options on mount#172
bigbigxu merged 1 commit intoCurvineIO:mainfrom
zuston:mountOptions

Conversation

@zuston
Copy link
Copy Markdown
Contributor

@zuston zuston commented Aug 11, 2025

No description provided.

@zuston
Copy link
Copy Markdown
Contributor Author

zuston commented Aug 11, 2025

Could you help take a look? @bigbigxu I think I will add some tests to validate this. Do you have some existing tests that I can extend

@zuston zuston changed the title feat: Add more options on mount feat: add more options on mount Aug 11, 2025
pub fn new(id: u32, curvine_uri: String, ufs_uri: String, mnt_opt: MountOptions) -> Self {
let consistency_conf = mnt_opt.consistency_config.unwrap_or_default();
let strategy = ConsistencyStrategy::from(consistency_conf);
let block_size = mnt_opt.block_size.unwrap_or_default();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unwrap_or_default should not be used here, option should still be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If so, users should always specify this option when mounting

}

async fn get_mount_point(&mut self, ufs_base_uri: &String) -> FsResult<MountPointInfo> {
pub async fn get_mount_point_with_uri(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This interface has been reimplemented in #117

opts_builder = opts_builder.ttl_action(action);
}

opts_builder = opts_builder
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When parameters such as block_size are None, the configuration in the curvine-cluster.toml file should be used.

Copy link
Copy Markdown
Contributor Author

@zuston zuston Aug 12, 2025

Choose a reason for hiding this comment

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

From my perspective, the curvine-cluster.toml file specified at startup may be inconsistent with the master’s configuration, especially if some workers start later than others. According to the central control design, workers should always retrieve their configurations from the master.

Based on the above assumption, block_size should never be None; this is ensured by the master when retrieving the mount point information.

What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

However, the block size of the mount point can be left unset. If not set, the client configuration of the current node should be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes. I think the block size should always be set by the master side. If user don't specify this when mounting, master will set this according to the client conf in this master node.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes. I think the block size should always be set by the master side. If user don't specify this when mounting, master will set this according to the client conf in this master node.

Is this OK? @bigbigxu

@szbr9486
Copy link
Copy Markdown
Contributor

BTW, could you add the usage in doc

@szbr9486
Copy link
Copy Markdown
Contributor

BTW, could you add the usage in doc顺便说一句,你能在文档中添加用法吗

Or i open a issue to follow this question

@zuston
Copy link
Copy Markdown
Contributor Author

zuston commented Aug 12, 2025

BTW, could you add the usage in doc

Yes, this will be done after this PR is ok for all reviewer.

@bigbigxu bigbigxu merged commit 712c44f into CurvineIO:main Aug 12, 2025
2 of 3 checks passed
@zuston
Copy link
Copy Markdown
Contributor Author

zuston commented Aug 12, 2025

Aha, this PR haven't add test cases (but merged). I will add tests and doc in the later PRs. @bigbigxu

@zuston zuston deleted the mountOptions branch August 12, 2025 06:41
@lzjqsdd
Copy link
Copy Markdown
Member

lzjqsdd commented Aug 12, 2025

Aha, this PR haven't add test cases (but merged). I will add tests and doc in the later PRs. @bigbigxu

maybe we can use [WIP] state in pr title to if this item havn't completed 😁

@lzjqsdd lzjqsdd added the enhancement New feature or request label Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants