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

Xload prefetch initial code #1585

Open
wants to merge 55 commits into
base: feature/xload
Choose a base branch
from
Open

Conversation

souravgupta-msft
Copy link
Member

✅ What

Initial code for prefetch feature in Xload component

🤔 Why

Download all the blobs on mount

NOTE

Please note that this the first PR for prefetch. Below items are in TODO list and will be added in future PRs

  • Stats collection
  • Open, close, getAttr and read dir calls in xload component
  • UT and E2E
  • Retry mechanism

@@ -1080,14 +1079,14 @@ func (bb *BlockBlob) TruncateFile(name string, size int64) error {
blobClient := bb.Container.NewBlockBlobClient(blobName)

blkList := make([]string, 0)
id := base64.StdEncoding.EncodeToString(common.NewUUIDWithLength(16))
id := common.GetBlockID(16)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider declaring 16 as a constant. This approach would make it easier to update the value in the future, as it would only need to be changed in one place. Additionally, assigning it to a well-named constant would enhance readability for developers.

data: addr,
offset: 0,
length: 0,
id: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we omit explicit assignments for default values (index: 0, offset: 0, etc.) as they are implicit in Go. This could make the code cleaner unless clarity is the goal.

Token: marker,
})
if err != nil {
log.Err("remoteLister::process : Remote listing failed for %s [%s]", absPath, err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

If remote listing fails, should we continue from here?

// TODO:: xload : should we delete the files from local path
err := common.TempCacheCleanup(xl.path)
if err != nil {
log.Err("unable to clean xload local path [%s]", err.Error())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we return an error from this function? If not, can we remove the return type entirely since the function currently only returns nil.

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.

3 participants