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

feat: [object-store] re-export hyper #5389

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

ritchie46
Copy link
Contributor

Closes #5386

Are there any user-facing changes?

No

@github-actions github-actions bot added the object-store Object Store Interface label Feb 11, 2024
@@ -0,0 +1,3 @@
//! Re-exports of libraries used by `object_store`.
Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to add the license goop at the top to make CI happy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, right.

@@ -497,6 +497,7 @@ pub use tags::TagSet;
pub mod multipart;
mod parse;
mod util;
pub mod export;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a convention found in other crates, or do they re-export at the root?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could not really find a convention. In this book it is done in the root: https://www.lurklurk.org/effective-rust/re-export.html

Which should work fine for hyper, but might give you some collisions if a library has a name colliding with internal modules. No preference on my side. Shall I do it from root?

@tustvold
Copy link
Contributor

So I did some digging because reqwest, which is the client we are actually using, doesn't export hyper and I wondered why. I think it is something to do with the way that reqwest supports wasm32. This isn't an immediate issue, given wasm32 for HTTP stores is currently incomplete, but it does make me wonder if this is the correct way to go about this.

Perhaps something like #5345 might be what you are looking for?

@ritchie46
Copy link
Contributor Author

Perhaps something like #5345 might be what you are looking for?

Which is an object-store variant of hyper/http errors? Seems also good. Since reqwest doesn't export hyper, but you do use it for error handling. Do you ensure manually to keep the versions matched?

@tustvold
Copy link
Contributor

Do you ensure manually to keep the versions matched

Yes, and have tests that should catch any divergence. It is not an ideal situation, but it has worked so far

@ritchie46
Copy link
Contributor Author

In that case, I would love to piggy back on that via an export. 😜

@tustvold
Copy link
Contributor

Perhaps you might expand upon your use-case for downcasting the errors? It occurs to me that this is somewhat breaking encapsulation, and we might be able to expose this information in a better way, perhaps??

@ritchie46
Copy link
Contributor Author

ritchie46 commented Feb 12, 2024

Perhaps you might expand upon your use-case for downcasting the errors? It occurs to me that this is somewhat breaking encapsulation, and we might be able to expose this information in a better way, perhaps??

I want to get most information out of the errors and see how to handle upon those. I haven't got a clear answer to what exactly my solution is. But having access to the http errors for accessing s3 is very important to us as I want to be close to maximum concurrency without triggering those. We want to be very fast here and like to be able experiment with tuning settings. Ideally I want to dynamically tune the client settings later.

It occurs to me that this is somewhat breaking encapsulation

Why? We can already get the hyper error out. It is only more painful now because we need to match the exact version. It is the same argument for your usage with reqwest. It would have been easier if they exported hyper. This gives power-users the ability to tinker.

@tustvold
Copy link
Contributor

tustvold commented Feb 12, 2024

Why? We can already get the hyper error out. It is only more painful now because we need to match the exact version

Because there is no requirement that all or even any ObjectStore are implemented using hyper, and in fact we already have some that are not 😅. I accept that power users may want to tinker, but I guess I'm wondering if we really want to be encouraging this by making it a first-party API.

But having access to the http errors for accessing s3 is very important to us as I want to be close to maximum concurrency without triggering those.

Perhaps you could identify from the list of errors exposed by S3 what variant you are interested in intercepting. We automatically backoff on receiving a 503, which is what it will return if you exceed the maximum concurrency (unless you are really hammering it in which case it will just drop the connections).

@ritchie46
Copy link
Contributor Author

Because there is no requirement that all or even any ObjectStore are implemented using hyper,

I totally understand that. And I as a user am willing to pay that breaking change risk. ;)

So we could export it under an unstable flag. Or if you don't want to expose it in object-store also fine, then we copy the hyper version from object-store and do it manually. Either way, I want to tinker. ^^

Perhaps you could identify from the list of errors exposed by S3 what variant you are interested in intercepting.

I don't know yet. I always learn while tinkering. 😅. It isn't a joke, I cannot answer what I need exactly yet. Have to learn this as we go.

If we have found something that works good for us, we can push that back into object-store. I rather have that handled higher up.

@tustvold
Copy link
Contributor

In that case, how about we mark this PR as a draft, and revisit this once we have a better idea of the concrete use-cases for this feature?

@ritchie46
Copy link
Contributor Author

Sure, fine by me. 👍

@ritchie46 ritchie46 marked this pull request as draft February 12, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[object-store] re-export hyper
2 participants