GH-36770: [C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT_URL#36791
GH-36770: [C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT_URL#36791kou merged 1 commit intoapache:mainfrom
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or In the case of PARQUET issues on JIRA the title also supports: See also: |
|
|
kou
left a comment
There was a problem hiding this comment.
Could you add a test to cpp/src/arrow/filesystem/s3fs_test.cc?
cpp/src/arrow/filesystem/s3fs.cc
Outdated
There was a problem hiding this comment.
| if(endpoint_env.ok()) { | |
| if (endpoint_env.ok()) { |
cpp/src/arrow/filesystem/s3fs.cc
Outdated
There was a problem hiding this comment.
How about using AWS_ENDPOINT_URL instead of AWS_ENDPOINT because aws-sdk-cpp will support AWS_ENDPOINT_URL aws/aws-sdk-cpp#2587 ?
(How about accepting AWS_ENDPOINT_URL in arrow-rs too?)
There was a problem hiding this comment.
Could you use the same name for constant name?
For example, kAwsEndpointEnvVar for this case.
cpp/src/arrow/filesystem/s3fs.cc
Outdated
There was a problem hiding this comment.
Can we simplify this?
| options.endpoint_override = endpoint_env.MoveValueUnsafe(); | |
| options.endpoint_override = *endpoint_env; |
|
@kou thanks for your comments, I updated PR, please review again. |
--amend --amend
kou
left a comment
There was a problem hiding this comment.
+1
Could you update the PR title and description before we merge this?
|
|
1 similar comment
|
|
updated, thanks @kou |
|
Thanks. |
|
@assignUser We have #36791 (comment) message but we couldn't assign #36770 to abdmal automatically. |
Hi @kou, This is due to #34389, they will have to comment on the issue first. |
|
Ah! Thanks! |
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit f43bfd6. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…riable AWS_ENDPOINT_URL (apache#36791) ### Rationale for this change we need a way to read custom object storage (such as minio host or other s3-like storage). use environment variable `AWS_ENDPOINT_URL ` ### What changes are included in this PR? set variable endpoint_override according the environment variable ### Are these changes tested? unittest and tested on pyarrow ### Are there any user-facing changes? No * Closes: apache#36770 Authored-by: yiwei.wang <yiwei.wang@weride.ai> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
we need a way to read custom object storage (such as minio host or other s3-like storage).
use environment variable
AWS_ENDPOINT_URLWhat changes are included in this PR?
set variable endpoint_override according the environment variable
Are these changes tested?
unittest and tested on pyarrow
Are there any user-facing changes?
No