-
Notifications
You must be signed in to change notification settings - Fork 401
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
[#5507] feat(python): Support Azure blob storage for GVFS python client #5538
Conversation
This PR is not ready for review until #5508 is merged. |
@yuqi1129 can you please rebase PR. |
ops = infer_storage_options(storage_location) | ||
if "username" not in ops or "host" not in ops or "path" not in ops: | ||
raise GravitinoRuntimeException( | ||
f"Storage location:{storage_location} doesn't support now. as the username," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Username, host...are required"
actual_prefix = f"{StorageType.ABS.value}://{ops['username']}@{ops['host']}{ops['path']}" | ||
|
||
# For ABS, the actual path should be the same as the virtual path is like | ||
# 'wasbs//[email protected]/test_gvfs_catalog6588/test_gvfs_schema/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we specify "wasbs"? Also what's the meaning here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the code here, the logic is specific for paths with wasbs
prefixes.
f"Storage location:{storage_location} doesn't support now, the username," | ||
f"host and path are required in the storage location." | ||
) | ||
actual_prefix = f"{StorageType.ABS.value}://{ops['username']}@{ops['host']}{ops['path']}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add /
before {ops['path']}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually not, please see L611, other file system like HDFS, file also omit the trialling slash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean "before", not "after".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value of path
starts with /
.
What changes were proposed in this pull request?
Support GVFS python client to access ADSL fileset.
Why are the changes needed?
This is a subsequent PR for #5508
Fix: #5507
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
IT locally.