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.

Sign up for GitHub

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

Jump to bottom

GH-38844: [C++] S3FileSystem export s3 sdk config "use_virtual_addressing" to arrow::fs::S3Options #38858

Merged
merged 7 commits into from
Dec 5, 2023
Merged

GH-38844: [C++] S3FileSystem export s3 sdk config "use_virtual_addressing" to arrow::fs::S3Options #38858

merged 7 commits into from
Dec 5, 2023

Conversation

liangjiawei1110
Copy link
Contributor

@liangjiawei1110 liangjiawei1110 commented Nov 23, 2023 •
edited
Loading

Rationale for this change

See #38844 . we should export s3 sdk config "use_virtual_addressing" to arrow::fs::S3Options so that user can decide whether to use virtual hosted by themselves.

Are these changes tested?

I tested it with my own OSS(aliyun object store service) bucket.As the it is a S3 compatiable service, I think
these changes are covered by existing tests)

Are there any user-facing changes?

Closes: #38844

@github-actions GitHub Actions
Copy link

⚠️ GitHub issue #38844 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting review Awaiting review Component: C++ and removed awaiting review Awaiting review labels Nov 23, 2023
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Nov 23, 2023
@liangjiawei1110 liangjiawei1110 changed the title GH-38844: [C++] S3FileSystem export s3 sdk config "use_virtual_addressing" to arrow::f::S3Options GH-38844: [C++] S3FileSystem export s3 sdk config "use_virtual_addressing" to arrow::fs::S3Options Nov 24, 2023
felipecrv
felipecrv reviewed Nov 29, 2023
View reviewed changes
Copy link
Contributor

@felipecrv felipecrv left a comment

Choose a reason for hiding this comment

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

Can we call it force_virtual_hosting instead of "addressing" like the AWS docs do? [1] I see that the AWS SDK calls it "addressing", but we don't have to use this confusing name in the Arrow interface.

[1] https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html

@pitrou
Copy link
Member

pitrou commented Dec 5, 2023

The widely used boto3 library also uses the term "virtual addressing", so I think it's ok.

At worse we can improve the docstring to make things clearer.

pitrou
pitrou approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, thank you for this @liangjiawei1110 !

@pitrou pitrou merged commit 501b291 into apache:main Dec 5, 2023
33 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Dec 5, 2023
@pitrou pitrou mentioned this pull request Dec 5, 2023
Closed
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Dec 5, 2023
@conbench-apache-arrow conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 501b291.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

@pitrou pitrou mentioned this pull request Dec 7, 2023
Closed
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@liangjiawei1110 @pitrou @dgreiss
apacheGH-38844: [C++] S3FileSystem export s3 sdk config "use_virtual_…
453aee4 
…addressing" to arrow::fs::S3Options (apache#38858)

### Rationale for this change
See apache#38844 . we should export s3 sdk config "use_virtual_addressing" to arrow::fs::S3Options so that user can decide whether to use virtual hosted by themselves. 

### Are these changes tested?
I tested it with my own OSS(aliyun object store service) bucket.As the it is a S3 compatiable service, I think 
 these changes are covered by existing tests)

### Are there any user-facing changes?

Closes: apache#38844  

* Closes: apache#38844

Lead-authored-by: ljw <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Co-authored-by: jiawei liang <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felipecrv felipecrv felipecrv left review comments

@pitrou pitrou pitrou approved these changes

Assignees
No one assigned
Labels
awaiting committer review Awaiting committer review Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++] S3 filesystem have bugs on virtual hosted config
3 participants
@liangjiawei1110 @pitrou @felipecrv