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.

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

Fix making too many create directory call #2828

Merged
merged 17 commits into from
Oct 24, 2024

Conversation

dphulkar-msft
Copy link
Collaborator

@dphulkar-msft dphulkar-msft commented Oct 11, 2024

Issue:
In the folderCreationTracker.go file, we addressed an issue where multiple createDirectory REST calls were being sent for the same folder during the copy operation, particularly with deeply nested directory structures. This was caused by the StopTracking() function prematurely removing folder entries from the contents map, leading to redundant createDirectory requests.

Fix:
To resolve this issue while maintaining memory optimization (which was the original reason for removing entries), we have replaced the contents map with a trie data structure. The trie allows us to efficiently track folder creation and ensures that redundant requests are avoided, while also optimizing memory usage.

@dphulkar-msft
Copy link
Collaborator Author

@gapra-msft @adreed-msft
Feel free to suggest alternative approaches.
This is just the reference MR showcasing a simple fix for the issue.

@dphulkar-msft dphulkar-msft changed the title Fix avoid making too many create directory call Fix making too many create directory call Oct 11, 2024
@vibhansa-msft vibhansa-msft added this to the 10.27.0 milestone Oct 12, 2024
Copy link
Member

@adreed-msft adreed-msft left a comment

Choose a reason for hiding this comment

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

A couple concerns I thought of...

  1. This only affects folders we're directly tracking to transfer properties. What about those that we're not?
  2. The mention of the use of a Trie instead of a hashmap on Vikas' comment
  3. What about when we're not trying to persist directories at all? e.g. blob->file. We need to carefully track the existing directory structure for a valid implementation of this.

ste/xfer-anyToRemote-folder.go Outdated Show resolved Hide resolved
ste/folderCreationTracker.go Outdated Show resolved Hide resolved
ste/folderCreationTracker.go Outdated Show resolved Hide resolved
Copy link
Member

@adreed-msft adreed-msft left a comment

Choose a reason for hiding this comment

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

Apologies for the wall of text, I was reading the code and just more things kept popping into my head.

common/trieForDirPath.go Show resolved Hide resolved
common/trieForDirPath.go Outdated Show resolved Hide resolved
common/trieForDirPath.go Outdated Show resolved Hide resolved
common/trieForDirPath.go Outdated Show resolved Hide resolved
common/trieForDirPath.go Outdated Show resolved Hide resolved
@gapra-msft gapra-msft merged commit b462994 into main Oct 24, 2024
22 checks passed
gapra-msft added a commit that referenced this pull request Nov 12, 2024
gapra-msft added a commit that referenced this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants