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

Jump to bottom

Improve HDFS client get status not use mount info #16944

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

Improve HDFS client get status not use mount info #16944

wants to merge 3 commits into from

Conversation

flaming-archer
Copy link
Contributor

What changes are proposed in this pull request?

like #16893 HDFS client get status not use mount info too

Why are the changes needed?

Does this PR introduce any user facing changes?

@alluxio-bot
Copy link
Contributor

Automated checks report:

  • Commits associated with Github account: PASS
  • PR title follows the conventions: FAIL
    • The title of the PR does not pass all the checks. Please fix the following issues:
      • First word of title ("HDFS") is not an imperative verb. Please use one of the valid words

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

@flaming-archer flaming-archer changed the title HDFS client get status not use mount info Improve HDFS client get status not use mount info Feb 24, 2023
@alluxio-bot
Copy link
Contributor

Automated checks report:

  • Commits associated with Github account: PASS
  • PR title follows the conventions: PASS

All checks passed!

@alluxio-bot alluxio-bot added the API Change Changes covering public API label Feb 24, 2023
@flaming-archer
Copy link
Contributor Author

@elega @jiacheliu3 Pls take a look. at it . I think HDFS client get status can not use mount info too.

@jiacheliu3
Copy link
Contributor

@elega @jiacheliu3 Pls take a look. at it . I think HDFS client get status can not use mount info too.

I think this makes sense to me. Do you mind rebasing on the current master? I feel this can be a quick review because the logic is basically the same. Thanks!

@flaming-archer
Copy link
Contributor Author

@elega @jiacheliu3 Pls take a look. at it . I think HDFS client get status can not use mount info too.

I think this makes sense to me. Do you mind rebasing on the current master? I feel this can be a quick review because the logic is basically the same. Thanks!

Done, please help to review.

This was referenced Mar 3, 2023
Merged
Open
elega
elega reviewed Mar 8, 2023
View reviewed changes
Copy link
Contributor

@elega elega left a comment

Choose a reason for hiding this comment

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

lgtm and i left some nit picky comments

shell/src/main/java/alluxio/cli/fs/command/StatCommand.java
@@ -55,6 +56,14 @@ public final class StatCommand extends AbstractFileSystemCommand {
.required(false)
.desc("specify a file by file-id")
.build();
private static final Option OMIT_MOUNT_INFO =
Option.builder("m")
Copy link
Contributor

Choose a reason for hiding this comment

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

the short option is confusing...
m as in mount? can probably use "xm" as in eXclude Mount info instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#16893

It is consistent with this. He said to use this. @jiacheliu3 .hh

shell/src/main/java/alluxio/cli/fs/command/StatCommand.java Outdated Show resolved Hide resolved
core/common/src/main/java/alluxio/conf/PropertyKey.java Outdated Show resolved Hide resolved
alluxio-bot pushed a commit that referenced this pull request Mar 9, 2023
@flaming-archer
Support getBlockInfo excluding mount-related info
2766440 
### What changes are proposed in this pull request?
Like #16893 and
#16944,
Before blockinfo will use mountInfo, so it should not use mountInfo
after set excludeMountInfo to true.

This purpose is also to improve rpc performance with HDFS client.

pr-link: #17006
change-id: cid-40e4961f7fbeee067d0b8ff1bc7d927d11b9745c
@flaming-archer flaming-archer requested review from elega and removed request for jiacheliu3 March 10, 2023 08:41
jiacheliu3 pushed a commit to jiacheliu3/alluxio that referenced this pull request May 17, 2023
@flaming-archer @jiacheliu3
Support getBlockInfo excluding mount-related info
aa51280 
### What changes are proposed in this pull request?
Like Alluxio#16893 and
Alluxio#16944,
Before blockinfo will use mountInfo, so it should not use mountInfo
after set excludeMountInfo to true.

This purpose is also to improve rpc performance with HDFS client.

pr-link: Alluxio#17006
change-id: cid-40e4961f7fbeee067d0b8ff1bc7d927d11b9745c
jiacheliu3 pushed a commit to jiacheliu3/alluxio that referenced this pull request May 17, 2023
@flaming-archer @jiacheliu3
Support getBlockInfo excluding mount-related info
c8b7d64 
### What changes are proposed in this pull request?
Like Alluxio#16893 and
Alluxio#16944,
Before blockinfo will use mountInfo, so it should not use mountInfo
after set excludeMountInfo to true.

This purpose is also to improve rpc performance with HDFS client.

pr-link: Alluxio#17006
change-id: cid-40e4961f7fbeee067d0b8ff1bc7d927d11b9745c
@github-actions
Copy link

github-actions bot commented Jun 6, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The PR/Issue does not have recent activities and will be closed automatically label Jun 6, 2023
@github-actions github-actions bot removed the stale The PR/Issue does not have recent activities and will be closed automatically label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elega elega Awaiting requested review from elega

Assignees

@tcrain tcrain

Labels
API Change Changes covering public API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@flaming-archer @alluxio-bot @jiacheliu3 @elega @tcrain