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

objstore : implement Baidu BOS #4506

Merged
merged 3 commits into from
Aug 24, 2021
Merged

objstore : implement Baidu BOS #4506

merged 3 commits into from
Aug 24, 2021

Conversation

yahaa
Copy link
Contributor

@yahaa yahaa commented Aug 1, 2021 •
edited
Loading

Try to reimplement Baidu BOS objstore.

Reference: #1329

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

# prepare some envs
$ export BOS_BUCKET=your_bos_bucket BOS_ENDPOINT=endpoint BOS_ACCESS_KEY=your_baidu_account_access_key BOS_SECRET_KEY=your_baidu_account_secret_key THANOS_ALLOW_EXISTING_BUCKET_USE=true THANOS_TEST_OBJSTORE_SKIP=GCS,S3,AZURE,SWIFT,COS,ALIYUNOSS

# run e2etest
$  go test -v ./pkg/objstore/...

@yahaa yahaa force-pushed the add-baidu-bos branch 2 times, most recently from 8bc3703 to 28b07e9 Compare August 1, 2021 13:59
@yahaa
Copy link
Contributor Author

yahaa commented Aug 1, 2021

@bwplotka @jojohappy Please help review this PR, thanks!

@yahaa yahaa force-pushed the add-baidu-bos branch 6 times, most recently from 6f20f45 to 0eb9efa Compare August 2, 2021 11:35
GiedriusS
GiedriusS reviewed Aug 3, 2021
View reviewed changes
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

There's no context.Context support? :(

pkg/objstore/bos/bos.go Outdated Show resolved Hide resolved
pkg/objstore/bos/bos.go Outdated Show resolved Hide resolved
pkg/objstore/bos/bos.go Outdated Show resolved Hide resolved
pkg/objstore/bos/bos.go Show resolved Hide resolved
pkg/objstore/bos/bos.go Outdated Show resolved Hide resolved
pkg/objstore/bos/bos_test.go Outdated Show resolved Hide resolved
@yahaa yahaa requested a review from GiedriusS August 3, 2021 12:22
bwplotka
bwplotka reviewed Aug 4, 2021
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks for this! Some comments, but generally looks amazing!

pkg/objstore/bos/bos.go Show resolved Hide resolved
pkg/objstore/bos/bos_test.go Outdated Show resolved Hide resolved
pkg/objstore/objtesting/foreach.go Outdated Show resolved Hide resolved
pkg/testutil/testutil.go Show resolved Hide resolved
@yahaa yahaa force-pushed the add-baidu-bos branch 5 times, most recently from 87ee39b to 0992c35 Compare August 4, 2021 15:38
@yahaa yahaa requested a review from bwplotka August 5, 2021 11:31
bwplotka
bwplotka previously approved these changes Aug 17, 2021
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing. LGTM, only suggestion towards documentation nits.

Thanks!

docs/storage.md Outdated Show resolved Hide resolved
docs/storage.md Outdated Show resolved Hide resolved
docs/storage.md Outdated Show resolved Hide resolved
pkg/objstore/bos/bos.go Outdated Show resolved Hide resolved
@yahaa
objstore : implement Baidu BOS
68c9388 
Signed-off-by: yahaa <1477765176@qq.com>
@yahaa
fmt docs/storage.md
4641161 
Signed-off-by: yahaa <1477765176@qq.com>
@yeya24
Copy link
Contributor

yeya24 commented Aug 24, 2021 •
edited
Loading

@yahaa Can you please resolve the changelog conflicts?

@yahaa
Copy link
Contributor Author

yahaa commented Aug 24, 2021

@yahaa Can you please resolve the changelog conflicts?

Ok i have resolved the conflicts.😁

bwplotka
bwplotka approved these changes Aug 24, 2021
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing work! Clean, straightforward.

My instinct tells me we might have problem with BOS naming too - it might be bit cryptic, but since we allowed COS already, let's go for it 👍🏽 Thanks!

@bwplotka bwplotka merged commit 0659ec0 into thanos-io:main Aug 24, 2021
@yahaa yahaa mentioned this pull request Oct 20, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift sonatype-lift left review comments

@bwplotka bwplotka bwplotka approved these changes

@GiedriusS GiedriusS Awaiting requested review from GiedriusS

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yahaa @yeya24 @GiedriusS @bwplotka