platform: add aws s3 storage #99

zojw · 2021-11-12T10:09:57Z

move demo-1's aws s3 code to here and adapt new Storage trait

tisonkun

Thanks for your contribution @zojw ! Comments inline.

Apart of comments I'd like to know why remote storage stay under storage crate and s3 storage stay under platform crate. It looks like we divide project layout with two nonorthogonal dimensions.

src/platform/src/aws/s3/mod.rs

src/storage/src/error.rs

tools/ci/licenserc.yml

src/storage/src/bucket.rs

zojw · 2021-11-12T10:48:19Z

Thanks for your contribution @zojw ! Comments inline.

Apart of comments I'd like to know why remote storage stay under storage crate and s3 storage stay under platform crate. It looks like we divide project layout with two nonorthogonal dimensions.

Thanks for review :) @tisonkun

add s3 storage under platform is introduced by another PR's comment #86 (comment)

for my understanding, in the storage crate, we only provide default implement (memory, remote, fs..), for third-part storage, users can implement it by their self's crate. platform maybe can be viewed as one third-part implement~

huachaohuang · 2021-11-12T10:53:39Z

Apart of comments I'd like to know why remote storage stay under storage crate and s3 storage stay under platform crate. It looks like we divide project layout with two nonorthogonal dimensions.

@tisonkun I think the discussion started here. My mindset is to put things that don't rely on third-party services in the component crate, and put vendor-related things in the platform together.

@zojw I further suggest that we make aws a sub-crate under platform. Then we may use features to conditional include different vendors.

src/storage/src/mem/bucket.rs

src/platform/src/aws/s3/bucket.rs

zojw · 2021-11-16T05:57:15Z

@tisonkun @huachaohuang addressed some comments, PTAL if free, thanks~

src/platform/aws/src/s3/bucket.rs

huachaohuang

I see this is your first PR, let work together to improve it a little bit :)

src/platform/aws/src/s3/bucket.rs

src/platform/aws/src/s3/object.rs

src/platform/aws/src/s3/storage.rs

huachaohuang · 2021-11-16T07:51:32Z

src/platform/aws/src/s3/storage.rs

+            .map_err(Error::from)
+    }
+
+    async fn create_new_bucket(&self, name: &str) -> Result<()> {


If you move it to an S3Client, you can rename it to create_bucket. "create" and "new" are redundant, IMO.

src/platform/aws/src/s3/storage.rs

huachaohuang · 2021-11-16T08:43:56Z

src/platform/aws/src/s3/error.rs

+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum Error {


We may define some common errors instead of simply exporting all these errors in the future. I think users have no idea how to handle these individual errors. But we can leave it this way for now, IMO.

+1 maybe we can refactor this later, for the platform IMHO, maybe we need still need an error mapping between the vendor's error and engula's error, but it should not be one-one mapping, we can hide some user not care detail, but it seems highly related to upper layer usage, maybe we can do it when upper layer and vendor's logic be clear :)

src/platform/aws/Cargo.toml

huachaohuang · 2021-11-17T02:58:27Z

@zojw I see your PR doesn't compile because of something related to a 'static lifetime. It's OK to specify 'static lifetime bound in trait. I think this post is helpful.

src/platform/aws/src/s3/mod.rs

src/platform/aws/src/s3/client.rs

src/platform/aws/src/s3/storage.rs

tisonkun · 2021-11-17T03:13:37Z

@zojw I see your PR doesn't compile because of something related to a 'static lifetime. It's OK to specify 'static lifetime bound in trait. I think this post is helpful.

@huachaohuang you can click the Approve and run button above to let ci do checks for you.

huachaohuang · 2021-11-17T04:08:53Z

@huachaohuang you can click the Approve and run button above to let ci do checks for you.

Yeah, I have done that before. But it seems that I need to click that every time it is updated.

tisonkun · 2021-11-17T04:12:37Z

@huachaohuang you can do it every time give the first-time contribution a review or when you think it ok to test :)

Anyway, tests should be run before you merge the pull request. This restriction is for first-time contribution or on a fork only, you can check out this post.

src/platform/aws/src/s3/storage.rs

src/platform/aws/Cargo.toml

Signed-off-by: zojw <robi@hey.com>

tisonkun

LGTM. The two outstanding comments aren't blockers IMO.

huachaohuang · 2021-11-17T11:51:52Z

@zojw please let me know if it is ready.

Signed-off-by: zojw <robi@hey.com>

zojw · 2021-11-17T11:55:29Z

@zojw please let me know if it is ready.

except #99 (comment) need some discuss, others are ready @huachaohuang

huachaohuang

Rest LGTM except for https://github.com/engula/engula/pull/99/files#r751275597

huachaohuang · 2021-11-17T14:15:29Z

src/platform/aws/src/s3/bucket.rs

+            return Ok(());
+        }
+
+        while data.len() > UPLOAD_PART_SIZE * 2 {


I think the idea is to upload UPLOAD_PART_SIZE at a time instead of UPLOAD_PART_SIZE * 2?

Signed-off-by: zojw <robi@hey.com>

huachaohuang

Everything looks great now. @zojw @tisonkun thanks for your contribution. I think we have some very fruitful discussions in this PR!

Signed-off-by: zojw <robi@hey.com>

tisonkun reviewed Nov 12, 2021

View reviewed changes

src/platform/src/aws/s3/mod.rs Outdated Show resolved Hide resolved

src/storage/src/error.rs Outdated Show resolved Hide resolved

tools/ci/licenserc.yml Outdated Show resolved Hide resolved

src/storage/src/bucket.rs Outdated Show resolved Hide resolved

huachaohuang reviewed Nov 12, 2021

View reviewed changes

src/storage/src/mem/bucket.rs Outdated Show resolved Hide resolved

huachaohuang reviewed Nov 12, 2021

View reviewed changes

src/platform/src/aws/s3/bucket.rs Outdated Show resolved Hide resolved

This was referenced Nov 14, 2021

journal: refactor traits with associated types #107

Merged

storage: refactor traits and implementations #108

Merged

zojw force-pushed the s3-storage branch from 80fdd37 to 568d3d3 Compare November 15, 2021 12:57

zojw marked this pull request as draft November 15, 2021 12:59

zojw marked this pull request as ready for review November 16, 2021 05:55

zojw mentioned this pull request Nov 16, 2021

Investigate how to test with s3 #109

Closed

tisonkun reviewed Nov 16, 2021

View reviewed changes

src/platform/aws/src/s3/bucket.rs Show resolved Hide resolved

huachaohuang reviewed Nov 16, 2021

View reviewed changes

src/platform/aws/Cargo.toml Outdated Show resolved Hide resolved

zojw mentioned this pull request Nov 16, 2021

Setup workflow to check unnecessary dependencies #110

Closed

tisonkun reviewed Nov 17, 2021

View reviewed changes

src/platform/aws/src/s3/mod.rs Outdated Show resolved Hide resolved

src/platform/aws/src/s3/client.rs Outdated Show resolved Hide resolved

src/platform/aws/src/s3/storage.rs Outdated Show resolved Hide resolved

zojw force-pushed the s3-storage branch from c888e33 to 8186a60 Compare November 17, 2021 03:36

tisonkun reviewed Nov 17, 2021

View reviewed changes

src/platform/aws/src/s3/storage.rs Outdated Show resolved Hide resolved

tisonkun mentioned this pull request Nov 17, 2021

fix aduit vulnerability and warning #112

Merged

tisonkun reviewed Nov 17, 2021

View reviewed changes

src/platform/aws/Cargo.toml Outdated Show resolved Hide resolved

zojw added 4 commits November 17, 2021 19:39

impl storage fo s3-platform

62c4cb2

Signed-off-by: zojw <robi@hey.com>

fix:avoid collect

e371de1

Signed-off-by: zojw <robi@hey.com>

backup

f325661

backup2

414b71e

Signed-off-by: zojw <robi@hey.com>

zojw added 11 commits November 17, 2021 19:39

let aws be standelone crate

c4c0c0d

Signed-off-by: zojw <robi@hey.com>

refine license check expression

1665243

Signed-off-by: zojw <robi@hey.com>

simplify map_err

6852d5b

fix delete bucket has 1000+ object

8a0003f

Signed-off-by: zojw <robi@hey.com>

revert unrelated modification

3729277

Signed-off-by: zojw <robi@hey.com>

use implicitly convert in return err

11c4f27

Signed-off-by: zojw <robi@hey.com>

fix name question

dce1869

Signed-off-by: zojw <robi@hey.com>

fix split_into/read_at/unused_dep

d721af5

Signed-off-by: zojw <robi@hey.com>

support inject mock client instead of provided a mocked client

90b81dd

Signed-off-by: zojw <robi@hey.com>

remove useless use declare

5d7a028

Signed-off-by: zojw <robi@hey.com>

upgrade platform module ver to 0.2.0

7c7b8d1

Signed-off-by: zojw <robi@hey.com>

zojw force-pushed the s3-storage branch from f0ee69d to 7c7b8d1 Compare November 17, 2021 11:40

tisonkun previously approved these changes Nov 17, 2021

View reviewed changes

remove more unused dep

bbdca0d

Signed-off-by: zojw <robi@hey.com>

zojw dismissed tisonkun’s stale review via bbdca0d November 17, 2021 11:52

huachaohuang reviewed Nov 17, 2021

View reviewed changes

zojw added 2 commits November 17, 2021 22:18

keep delete_bucket disallow non-empty bucket

2f9e310

Signed-off-by: zojw <robi@hey.com>

fix: upload 1 part if bigger than 2 parts size

eb86980

Signed-off-by: zojw <robi@hey.com>

huachaohuang previously approved these changes Nov 17, 2021

View reviewed changes

fix fmt

27382c3

Signed-off-by: zojw <robi@hey.com>

zojw dismissed huachaohuang’s stale review via 27382c3 November 17, 2021 15:00

huachaohuang merged commit bd69f2e into engula:main Nov 17, 2021

zojw deleted the s3-storage branch November 17, 2021 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

platform: add aws s3 storage #99

platform: add aws s3 storage #99

zojw commented Nov 12, 2021

tisonkun left a comment

zojw commented Nov 12, 2021 •

edited

huachaohuang commented Nov 12, 2021

zojw commented Nov 16, 2021

huachaohuang left a comment

huachaohuang Nov 16, 2021

huachaohuang Nov 16, 2021

zojw Nov 16, 2021 •

edited

huachaohuang commented Nov 17, 2021

tisonkun commented Nov 17, 2021 •

edited

huachaohuang commented Nov 17, 2021

tisonkun commented Nov 17, 2021

tisonkun left a comment

huachaohuang commented Nov 17, 2021

zojw commented Nov 17, 2021 •

edited

huachaohuang left a comment

huachaohuang Nov 17, 2021

huachaohuang left a comment

platform: add aws s3 storage #99

platform: add aws s3 storage #99

Conversation

zojw commented Nov 12, 2021

tisonkun left a comment

Choose a reason for hiding this comment

zojw commented Nov 12, 2021 • edited

huachaohuang commented Nov 12, 2021

zojw commented Nov 16, 2021

huachaohuang left a comment

Choose a reason for hiding this comment

huachaohuang Nov 16, 2021

Choose a reason for hiding this comment

huachaohuang Nov 16, 2021

Choose a reason for hiding this comment

zojw Nov 16, 2021 • edited

Choose a reason for hiding this comment

huachaohuang commented Nov 17, 2021

tisonkun commented Nov 17, 2021 • edited

huachaohuang commented Nov 17, 2021

tisonkun commented Nov 17, 2021

tisonkun left a comment

Choose a reason for hiding this comment

huachaohuang commented Nov 17, 2021

zojw commented Nov 17, 2021 • edited

huachaohuang left a comment

Choose a reason for hiding this comment

huachaohuang Nov 17, 2021

Choose a reason for hiding this comment

huachaohuang left a comment

Choose a reason for hiding this comment

zojw commented Nov 12, 2021 •

edited

zojw Nov 16, 2021 •

edited

tisonkun commented Nov 17, 2021 •

edited

zojw commented Nov 17, 2021 •

edited