Skip to content

replace rusoto_s3: part 2 (last part) #245

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
Merged
houqp merged 2 commits into from
Jan 19, 2023
Merged

replace rusoto_s3: part 2 (last part) #245

houqp merged 2 commits into from
Jan 19, 2023

Conversation

@jychen7
Copy link
Collaborator

@jychen7 jychen7 commented Jan 18, 2023 •
edited
Loading

Why

step 2 of #240: replace rusoto_s3 (in use_memory_table: true path) with S3 client from object_store (s3://)

rusoto_s3 is used in columnq::io::s3 and columnq::io::s3 is used in two places:

  • columnq::partitions_from_table_source
  • columnq::table::delta

we would like to remove rusoto_s3 and switch to object_store, so can unify S3/GCS/Azure in future

How

following part 1 (#244), this PR (part 2) will focus on migrating columnq::table::delta and delete columnq::io::s3

this PR also remove crate rusoto_xxx completely and close #243

Test

Rely on existing unit tests in

roapi/columnq/src/table/delta.rs

Lines 131 to 132 in 1e3644e

#[tokio::test]
async fn load_delta_as_memtable() -> Result<(), ColumnQError> {

roapi/columnq/src/table/delta.rs

Lines 151 to 152 in 1e3644e

#[tokio::test]
async fn load_delta_as_delta_source() -> Result<(), ColumnQError> {

@jychen7 jychen7 requested a review from houqp January 18, 2023 14:08
@jychen7 jychen7 mentioned this pull request Jan 18, 2023
Closed
13 tasks
@jychen7 jychen7 changed the title replace rusoto: part 2 (last part) replace rusoto_s3: part 2 (last part) Jan 18, 2023
jychen7
jychen7 commented Jan 18, 2023
View reviewed changes
columnq/Cargo.toml

tokio = { version = "1", features = ["rt-multi-thread"] }
futures = "0.3"
# S3
Copy link
Collaborator Author

@jychen7 jychen7 Jan 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

now s3 is refactor, how about I create a release PR for 0.8.2 after this PR is approved and merged.

after we adds GCS and Azure support, we can release 0.9.0? What do you think?

Copy link
Member

@houqp houqp Jan 19, 2023

Choose a reason for hiding this comment

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

that sounds good to me, let's do it!

@houqp
Copy link
Member

houqp commented Jan 19, 2023 •
edited
Loading

One thing I forgot to mention in the previous PR review, we should enable the aws-profile feature in the object store crate because using profile to inject credential is a very common pattern in AWS land.

Without this change, it will break the existing users that are using AWS profiles.

@houqp houqp merged commit bd168d9 into roapi:main Jan 19, 2023
@jychen7 jychen7 deleted the 240-replace-rusoto-part-2 branch January 19, 2023 14:15
@jychen7 jychen7 mentioned this pull request Jan 19, 2023
Merged
@jychen7
Copy link
Collaborator Author

jychen7 commented Jan 19, 2023

One thing I forgot to mention in the previous PR review, we should enable the aws-profile feature in the object store crate because using profile to inject credential is a very common pattern in AWS land.

good point, improve here: #247

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@houqp houqp houqp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RUSTSEC-2022-0071: Rusoto is unmaintained

2 participants

@jychen7 @houqp