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.

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

add endpoint url param for s3 dal #2555

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

dantengsky
Copy link
Member

@dantengsky dantengsky commented Oct 31, 2021

I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/

Summary

  • add endpoint_url to S3StorageConfig
  • using custom Region if endpoint_url is enabled
  • use minio in statless-standalone-test(ubuntu-latest only)
  • minor code gc

@ZhiHanZ thanks for the github action code snippets , it works really well! but

  • I failed to use it in config macos-latest, no docker there. thus minio is enabled only for ubuntu-latest. if there are better solutions, please let me know.
  • workflows/stateless-tests-standalone.yml has been tweaked to enable minio. there are three extra steps doing nothing in macos-latest... not as elegant as before. is there a better way to do this? or shall we improve it later?

@drmingdrmer thanks for suggesting using moto as the mock service, but @wubx kindly reminds me that there might be real cases of using minio later, and mocking is only used in integration tests in this PR, thus minio is taken.

part of

Changelog

  • New Feature
  • Not for changelog (changelog entry is not required)

Related Issues

part of issue #1780 #2561

Test Plan

Unit Tests

Stateless Tests

@databend-bot databend-bot added pr-feature this PR introduces a new feature to the codebase pr-not-for-changelog labels Oct 31, 2021
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@dantengsky dantengsky force-pushed the feature-s3-endpoint-url branch from c41adb0 to 4d4a323 Compare October 31, 2021 17:03
@dantengsky dantengsky added the v0.6 v0.6 version label Nov 1, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2021

Codecov Report

Merging #2555 (5daa9a1) into main (a398ed6) will decrease coverage by 0%.
The diff coverage is 32%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2555   +/-   ##
=====================================
- Coverage     72%     72%   -1%     
=====================================
  Files        646     644    -2     
  Lines      36383   36378    -5     
=====================================
- Hits       26310   26299   -11     
- Misses     10073   10079    +6     
Impacted Files Coverage Δ
common/dal/src/data_accessor.rs 91% <ø> (ø)
common/dal/src/impls/aws_s3/s3.rs 0% <0%> (ø)
...ommon/dal/src/impls/aws_s3/s3_input_stream_test.rs 0% <0%> (ø)
...on/dal/src/impls/azure_blob/azure_blob_accessor.rs 0% <ø> (ø)
common/dal/src/impls/local.rs 75% <ø> (+3%) ⬆️
query/src/datasources/common/dal_builder.rs 42% <0%> (-2%) ⬇️
query/src/datasources/table/fuse/io/mod.rs 100% <ø> (ø)
.../src/datasources/table/fuse/meta/table_snapshot.rs 56% <ø> (ø)
...y/src/datasources/table/fuse/index/min_max_test.rs 91% <71%> (-2%) ⬇️
query/src/configs/config_storage.rs 87% <100%> (+<1%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a398ed6...5daa9a1. Read the comment docs.

@dantengsky dantengsky force-pushed the feature-s3-endpoint-url branch 4 times, most recently from daf408f to 2f72b48 Compare November 1, 2021 14:07
- add endpoint_url config
- using custom region if endpoint_url is enabled
- add minio to CI
- use minio in statless-standalone-test(ubuntu-latest)
- minor code gc
@dantengsky dantengsky force-pushed the feature-s3-endpoint-url branch from 2f72b48 to 0fd58fe Compare November 2, 2021 01:38
@dantengsky dantengsky marked this pull request as ready for review November 2, 2021 02:58
@BohuTANG BohuTANG requested a review from ZhiHanZ November 2, 2021 03:05
@BohuTANG
Copy link
Member

BohuTANG commented Nov 2, 2021

Cool~
What happens if we run make stateless-test locally?

@BohuTANG
Copy link
Member

BohuTANG commented Nov 2, 2021

Cool~ What happens if we run make stateless-test locally?

Got it, the github stateless test sets the s3 as the default storage :)

@BohuTANG
Copy link
Member

BohuTANG commented Nov 2, 2021

but @wubx kindly reminds me that there might be real cases of using minio later, and mocking is only used in integration tests in this PR, thus minio is taken.

This is stateful test(issue #2516) not stateless test, cc @wubx

@PsiACE
Copy link
Member

PsiACE commented Nov 2, 2021

LGTM, although stateless-test doesn't necessarily require s3 , it can help us verify that it works.

Perhaps we also need to provide such support for azure blob or other later?

@drmingdrmer
Copy link
Member

Does it mean we mac users can only test s3 backend in a CI?

@dantengsky
Copy link
Member Author

Perhaps we also need to provide such support for azure blob or other later?

yes, I think we need them

@dantengsky
Copy link
Member Author

Does it mean we mac users can only test s3 backend in a CI?

for "auto" tests, unfortunately, yes. we can improve this eventually I think.

but by exporting the env vars before cargo test make stateless-test..., something like this:

          export STORAGE_TYPE=s3
          export S3_STORAGE_BUCKET=testbucket
          export S3_STORAGE_REGION=us-east-1
          export S3_STORAGE_ENDPOINT_URL=http://127.0.0.1:9000
          export S3_STORAGE_ACCESS_KEY_ID=minioadmin
          export S3_STORAGE_SECRET_ACCESS_KEY=minioadmin
          bash ./scripts/ci/ci-run-stateless-tests-standalone.sh

we can test s3 backend locally (but manually setup)

@drmingdrmer
Copy link
Member

Does it mean we mac users can only test s3 backend in a CI?

for "auto" tests, unfortunately, yes. we can improve this eventually I think.

but by exporting the env vars before cargo test make stateless-test..., something like this:

          export STORAGE_TYPE=s3
          export S3_STORAGE_BUCKET=testbucket
          export S3_STORAGE_REGION=us-east-1
          export S3_STORAGE_ENDPOINT_URL=http://127.0.0.1:9000
          export S3_STORAGE_ACCESS_KEY_ID=minioadmin
          export S3_STORAGE_SECRET_ACCESS_KEY=minioadmin
          bash ./scripts/ci/ci-run-stateless-tests-standalone.sh

we can test s3 backend locally (but manually setup)

What about adding these config to a script like stateless-test-s3 and let user run it by just one click?

@dantengsky
Copy link
Member Author

What about adding these config to a script like stateless-test-s3 and let user run it by just one click?

good idea! like ci-run-stateless-tests-cluster-tls.sh.

@drmingdrmer
Copy link
Member

/lgtm

@databend-bot
Copy link
Member

Approved! Thank you for the PR @dantengsky

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@BohuTANG BohuTANG merged commit b9eb711 into databendlabs:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase v0.6 v0.6 version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants