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

refactor: Integrate with opendal for s3 #1412

Merged
merged 10 commits into from
Dec 11, 2022
Merged

refactor: Integrate with opendal for s3 #1412

merged 10 commits into from
Dec 11, 2022

Conversation

Xuanwo
Copy link
Collaborator

@Xuanwo Xuanwo commented Nov 16, 2022

Signed-off-by: Xuanwo github@xuanwo.io

Hi, this is a quick demo for #1404. This PR intends to show what opendal's API looks like and how opendal will work with sccache. And give the whole community a chance to try opendal.

This PR only changes the s3 implementation. In face, we can implement Cache for opendal::Operator so that we don't need to implement Cache for different storage backends anymore.

Any comments are welcome! Thank you so much for your attention and participation.

Fix #1386
Fix #1404
Fix #633

@Xuanwo

This comment was marked as off-topic.

@messense
Copy link
Contributor

messense commented Nov 17, 2022

sccache currently requires MSRV 1.58.0, so opendal also need to support Rust 1.58.0 unless the maintainers are willing to update to a new MSRV policy.

bincode 2.0.0-rc2 uses the new weak dependency feature that was stabilized in Rust 1.60.0.

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Nov 17, 2022

sccache currently requires MSRV 1.58.0, so opendal also need to support Rust 1.58.0 unless the maintainers are willing to update to a new MSRV policy.

Thanks for explanation!

@sylvestre
Copy link
Collaborator

What is the MSRV for opendal?

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Nov 21, 2022

What is the MSRV for opendal?

opendal v0.20.1 can run on 1.60: apache/opendal#968

@sylvestre
Copy link
Collaborator

Needs rebasing now, thanks :)

Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo marked this pull request as ready for review November 27, 2022 11:39
@Xuanwo Xuanwo changed the title demo: How to integrate with opendal refactor: Integrate with opendal Nov 27, 2022
src/cache/s3.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2022

Codecov Report

Base: 23.61% // Head: 30.07% // Increases project coverage by +6.45% 🎉

Coverage data is based on head (d3d2e90) compared to base (a31cf64).
Patch coverage: 47.36% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1412      +/-   ##
==========================================
+ Coverage   23.61%   30.07%   +6.45%     
==========================================
  Files          49       49              
  Lines       22414    17409    -5005     
  Branches    10265     8305    -1960     
==========================================
- Hits         5294     5236      -58     
+ Misses      11528     6556    -4972     
- Partials     5592     5617      +25     
Impacted Files Coverage Δ
src/cache/s3.rs 29.88% <47.36%> (+11.99%) ⬆️
src/mock_command.rs 50.93% <0.00%> (-1.88%) ⬇️
src/util.rs 35.00% <0.00%> (-1.57%) ⬇️
src/lib.rs 9.33% <0.00%> (-1.46%) ⬇️
src/cache/cache.rs 37.23% <0.00%> (-1.26%) ⬇️
src/test/utils.rs 36.36% <0.00%> (-1.02%) ⬇️
src/dist/cache.rs 20.67% <0.00%> (-0.76%) ⬇️
src/cache/azure.rs 30.88% <0.00%> (-0.74%) ⬇️
src/compiler/c.rs 38.76% <0.00%> (-0.69%) ⬇️
src/compiler/args.rs 61.19% <0.00%> (-0.36%) ⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo changed the title refactor: Integrate with opendal refactor: Integrate with opendal for s3 Nov 28, 2022
@drahnr
Copy link
Collaborator

drahnr commented Dec 2, 2022

Sorry for the late chime:

There are a few risks associated with opendal, it..

  • .. depends on tokio, so we are signing up for that, not a big deal I think, we already use tokio heavily and I don't see that changing
  • .. has a lot backends we don't need, i.e. rocksdb - these are configurable via feature flags in main in opendal - and that'd be one condition to not inflate the compile time cost for zero benefit to sccache

Open questions:

  • Future proof-ness, how to assure continued maintainence effort?
  • What are the stability guarantees on an API level, are they fully semver compliant?
  • MSRV handling?

On the other side, we (as in sccache) gain quite a bit:

  • Simplified addition of more backends
  • The sampled code and documentation looks actually better than what we have right now
  • Shared load of maintainence web service API changes

My bottom line is a yay given the manpower that's available to maintain sccache, and assuming the open questions are addressed in a way that doesn't risk stability or breaking builds of sccache

@sylvestre
Copy link
Collaborator

Great summary, exactly how i feel...

maybe we could do a new release of sccache with the current code (and a few PR)
and take this into a new release

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Dec 2, 2022

  • .. has a lot backends we don't need, i.e. rocksdb - these are configurable via feature flags in main in opendal - and that'd be one condition to not
    inflate
    the compile time cost for zero benefit to sccache

Most backends like rocksdb are only included while the corresponding feature enabled like: https://opendal.databend.rs/opendal/#services-1

They will not add the compile time cost for sccache unless enabling them.

Services that only need HTTP client will be enabled by default (which includes services like s3, gcs, oss) because they will not introduce extra heavy dependences.

If this is a big concern to sccache, I'm willing to add new feature flags for them too.


Answers to open questions:

  • Future proof-ness, how to assure continued maintainence effort?

Yes. OpenDAL is the core dependence of databend and we will continue to maintain it. And we are kept enlarge our community to get more maintainers.

So far, we have two maintainers: @Xuanwo and @PsiACE.

  • What are the stability guarantees on an API level, are they fully semver compliant?

Yes, we adopt the semver compliant, all changes will be recorded at https://github.com/datafuselabs/opendal/blob/main/CHANGELOG.md

NOTE: OpenDAL is on the track on v0.X.Y now, we could introduce breaking changes between v0.X version.

We are working hard to make a v1.0 release.

  • MSRV handling?

OpenDAL's MSRV is 1.60 which is covered by our CI services: https://github.com/datafuselabs/opendal/blob/main/.github/workflows/ci.yml#L42

We will not change the MSRV before v1.0 versions.


maybe we could do a new release of sccache with the current code (and a few PR)

LGTM.

@drahnr
Copy link
Collaborator

drahnr commented Dec 2, 2022

@Xuanwo thank you for the prompt response, it addressed my concerns, for me it's good to go, right after:

@sylvestre we should cut a release before merging this PR as you suggested, just to get some experience/burning time with it

Signed-off-by: Xuanwo <github@xuanwo.io>
Signed-off-by: Xuanwo <github@xuanwo.io>
@drahnr
Copy link
Collaborator

drahnr commented Dec 10, 2022

@sylvestre I think it's time to merge

@sylvestre
Copy link
Collaborator

@Xuanwo you are going to update the various backend next, right ?

@Xuanwo
Copy link
Collaborator Author

Xuanwo commented Dec 11, 2022

@Xuanwo you are going to update the various backend next, right ?

Yes, I plan to migrate azure, gcs first and than adding new backends support.

@sylvestre
Copy link
Collaborator

Github is complaining about conflicts

@drahnr
Copy link
Collaborator

drahnr commented Dec 11, 2022

Not for me, merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants