Skip to content

refactor(services/azfile): Check if dir exists before create #3649

Closed
ZutJoe wants to merge 2 commits intoapache:mainfrom
ZutJoe:main
Closed

refactor(services/azfile): Check if dir exists before create #3649
ZutJoe wants to merge 2 commits intoapache:mainfrom
ZutJoe:main

Conversation

@ZutJoe
Copy link
Copy Markdown
Contributor

@ZutJoe ZutJoe commented Nov 22, 2023

refactor #3646

@ZutJoe ZutJoe requested a review from Xuanwo as a code owner November 22, 2023 10:38
@github-actions github-actions Bot added the releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" label Nov 22, 2023
@ZutJoe ZutJoe changed the title refactor(services/azfile): Check if dir exists before create (#3646) refactor(services/azfile): Check if dir exists before create Nov 22, 2023
}
for dir in dirs {
let resp = self.azfile_create_dir(dir).await?;
let resp = self.azfile_create_dir(p).await?;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change seems wrong.

Given this case: a/b/c:

  • create_dir("a/b/") failed since a/ doesn't exist.
  • create_dir("a/") succeed.
  • But write a/b/c still failed since a/b/ doesn't exist.

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.

fix

I'm not sure I got it right in #3646 , My English is not good

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for not make it clear in this issue.

The check means sending azfile_get_path_properties call before calling azfile_create_dir to avoid extra API calls.

Take a/b/c as an example:

  • Check if a/b/ exist -> No
  • Check if a/ exist -> No
  • Create a/
  • Create a/b/

In the best case: if a/b/ exists, we can avoid two APIs of create_dir.

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.

I think I understand. Thank you for your patience

I will close the pr

@ZutJoe ZutJoe closed this Nov 22, 2023
@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented Nov 22, 2023

Hi, does closing this PR mean you're abandoning the issue? It's OK to work on the same PR if you still have interest without closing it.

@ZutJoe
Copy link
Copy Markdown
Contributor Author

ZutJoe commented Nov 22, 2023

Hi, does closing this PR mean you're abandoning the issue? It's OK to work on the same PR if you still have interest without closing it.

I have submitted another PR at #3652 Or should I continue to submit in this PR

@Xuanwo
Copy link
Copy Markdown
Member

Xuanwo commented Nov 22, 2023

I have submitted another PR at #3652 Or should I continue to submit in this PR

It's better to continue work on the same PR. But since you have already start a new PR, we can move discussion there instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants