GH-36770: [C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT_URL #36791

adbmal · 2023-07-20T16:36:10Z

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: [C++] Use custom url for s3 using AWS_ENDPOINT_URL #36770

github-actions · 2023-07-20T16:36:36Z

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?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2023-07-20T16:39:50Z

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

kou

Could you add a test to cpp/src/arrow/filesystem/s3fs_test.cc?

kou · 2023-07-20T20:11:19Z

cpp/src/arrow/filesystem/s3fs.cc

@@ -337,6 +338,10 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
  } else {
    options.ConfigureDefaultCredentials();
  }
+  auto endpoint_env = arrow::internal::GetEnvVar(kDefaultEndpointEnvVar);
+  if(endpoint_env.ok()) {


Suggested change

if(endpoint_env.ok()) {

if (endpoint_env.ok()) {

kou · 2023-07-20T20:13:26Z

cpp/src/arrow/filesystem/s3fs.cc

@@ -127,6 +127,7 @@ using internal::ToAwsString;
 using internal::ToURLEncodedAwsString;

 static const char kSep = '/';
+constexpr char kDefaultEndpointEnvVar[] = "AWS_ENDPOINT";


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?)

Could you use the same name for constant name?
For example, kAwsEndpointEnvVar for this case.

kou · 2023-07-20T20:18:59Z

cpp/src/arrow/filesystem/s3fs.cc

@@ -337,6 +338,10 @@ Result<S3Options> S3Options::FromUri(const Uri& uri, std::string* out_path) {
  } else {
    options.ConfigureDefaultCredentials();
  }
+  auto endpoint_env = arrow::internal::GetEnvVar(kDefaultEndpointEnvVar);
+  if(endpoint_env.ok()) {
+    options.endpoint_override = endpoint_env.MoveValueUnsafe();


Can we simplify this?

Suggested change

options.endpoint_override = endpoint_env.MoveValueUnsafe();

options.endpoint_override = *endpoint_env;

adbmal · 2023-07-21T03:11:44Z

@kou thanks for your comments, I updated PR, please review again.

--amend --amend

kou

+1

Could you update the PR title and description before we merge this?

github-actions · 2023-07-21T07:24:57Z

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

github-actions · 2023-07-21T07:29:31Z

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

adbmal · 2023-07-21T07:29:47Z

+1

Could you update the PR title and description before we merge this?

updated, thanks @kou

kou · 2023-07-21T07:39:47Z

Thanks.
I've merged this. Could you add take only comment to #36770 to assign it to you?

kou · 2023-07-21T07:42:29Z

@assignUser We have #36791 (comment) message but we couldn't assign #36770 to abdmal automatically.

js8544 · 2023-07-21T08:11:07Z

@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.

kou · 2023-07-21T08:20:07Z

Ah! Thanks!

conbench-apache-arrow · 2023-07-29T10:15:44Z

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 <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added Component: C++ awaiting review Awaiting review labels Jul 20, 2023

adbmal changed the title ~~[C++] Use custom endpoint for s3 using AWS_ENDPOINT~~ [C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT Jul 20, 2023

adbmal changed the title ~~[C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT~~ GH-36770: [C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT Jul 20, 2023

kou reviewed Jul 20, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting review Awaiting review awaiting changes Awaiting changes labels Jul 20, 2023

adbmal force-pushed the main branch from 10f9f68 to 0d6d032 Compare July 21, 2023 03:30

[C++] Use custom endpoint for s3 using AWS_ENDPOINT

62c8882

--amend --amend

adbmal force-pushed the main branch from 0d6d032 to 62c8882 Compare July 21, 2023 04:17

kou approved these changes Jul 21, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 21, 2023

adbmal changed the title ~~GH-36770: [C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT~~ GH-36770: [C++] Use custom endpoint for s3 using environment variable AWS_ENDPOINT_URL Jul 21, 2023

kou merged commit f43bfd6 into apache:main Jul 21, 2023
36 checks passed

kou removed the awaiting merge Awaiting merge label Jul 21, 2023

messense mentioned this pull request Nov 10, 2023

[C++] Support service-specific endpoint for S3 using AWS_ENDPOINT_URL_S3 #38663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #36791

adbmal commented Jul 20, 2023 •

edited

Loading

github-actions bot commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

kou left a comment

kou Jul 20, 2023

kou Jul 20, 2023

kou Jul 20, 2023

kou Jul 20, 2023

adbmal commented Jul 21, 2023 •

edited

Loading

kou left a comment

github-actions bot commented Jul 21, 2023

github-actions bot commented Jul 21, 2023

adbmal commented Jul 21, 2023

kou commented Jul 21, 2023 •

edited

Loading

kou commented Jul 21, 2023

js8544 commented Jul 21, 2023

kou commented Jul 21, 2023

conbench-apache-arrow bot commented Jul 29, 2023

	options.endpoint_override = endpoint_env.MoveValueUnsafe();
	options.endpoint_override = *endpoint_env;

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 #36791

Conversation

adbmal commented Jul 20, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

kou left a comment

Choose a reason for hiding this comment

kou Jul 20, 2023

Choose a reason for hiding this comment

kou Jul 20, 2023

Choose a reason for hiding this comment

kou Jul 20, 2023

Choose a reason for hiding this comment

kou Jul 20, 2023

Choose a reason for hiding this comment

adbmal commented Jul 21, 2023 • edited Loading

kou left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 21, 2023

github-actions bot commented Jul 21, 2023

adbmal commented Jul 21, 2023

kou commented Jul 21, 2023 • edited Loading

kou commented Jul 21, 2023

js8544 commented Jul 21, 2023

kou commented Jul 21, 2023

conbench-apache-arrow bot commented Jul 29, 2023

adbmal commented Jul 20, 2023 •

edited

Loading

adbmal commented Jul 21, 2023 •

edited

Loading

kou commented Jul 21, 2023 •

edited

Loading