-
Notifications
You must be signed in to change notification settings - Fork 413
Support fsspec s3 addressing_style properties #2517
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
Conversation
| S3_SIGNER_ENDPOINT_DEFAULT = "v1/aws/s3/sign" | ||
| S3_ROLE_ARN = "s3.role-arn" | ||
| S3_ROLE_SESSION_NAME = "s3.role-session-name" | ||
| S3_FORCE_VIRTUAL_ADDRESSING = "s3.force-virtual-addressing" |
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 see we already have this one, should we re-use that one? I'm not sure why it isn't being used 🤔 Seems like a faulty refactor somewhere
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 see we already have this one, should we re-use that one? I'm not sure why it isn't being used 🤔 Seems like a faulty refactor somewhere
Because two different S3 dependency libraries are used, the configuration parameters for this in the two libraries are different.
|
@Fokko I modified and used the same parameter s3.force-virtual-addressing to changed addressing_style In fsspec impl, s3 api dependent s3fs -> aiobotocore, this lib use s3.addressing_style to control. But in pyarrow impl, s3 api dependent from pyarrow.fs import S3FileSystem, this lib use force-virtual-addressing to control. |
kevinjqliu
left a comment
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.
LGTM!
| config_kwargs["read_timeout"] = float(request_timeout) | ||
|
|
||
| if _force_virtual_addressing := properties.get(S3_FORCE_VIRTUAL_ADDRESSING): | ||
| config_kwargs["s3"] = {"addressing_style": "virtual"} |
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.
👍
config_kwargs is passed to botocore.client.Config
https://s3fs.readthedocs.io/en/latest/api.html#s3fs.core.S3FileSystem
botocore.client.Config expects botocore.client.Config in s3 dict
https://botocore.amazonaws.com/v1/documentation/api/latest/reference/config.html
|
Thanks for the fix @fcfangcc |
Rationale for this change
Impl #2516
Then changed it like
Are these changes tested?
Add test case for fsspec s3 addressing_style properties
Are there any user-facing changes?
The default behavior is not modified