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

Make coalesce_ranges and collect_bytes available for crate users #4784

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

sumerman
Copy link
Contributor

@sumerman sumerman commented Sep 6, 2023

Which issue does this PR close?

I didn't create an issue

Rationale for this change

Both coalesce_ranges and collect_bytes are useful for AsyncFileReader implementations that don't quite fit object_store interface, but address the same fundamental problem — fetching data from a remote location.

What changes are included in this PR?

coalesce_ranges, collect_bytes and OBJECT_STORE_COALESCE_DEFAULT are made pub use

Are there any user-facing changes?

@github-actions github-actions bot added the object-store Object Store Interface label Sep 6, 2023
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

#4786 should resolve the clippy failures

@sumerman
Copy link
Contributor Author

sumerman commented Sep 7, 2023

#4786 should resolve the clippy failures

Thank you! I've rebased my PR

@sumerman
Copy link
Contributor Author

sumerman commented Sep 7, 2023

Another change, that I believe would be useful for non-object_store uses, is switching to std::result::Result type for coalesce_ranges and allowing errors that aren't object_store::Error. WDYT?

@tustvold
Copy link
Contributor

tustvold commented Sep 7, 2023

Adding the error as a type parameter doesn't seem like it would be particularly controversial / complex. I am somewhat curious what you are intending to use this for though?

@sumerman
Copy link
Contributor Author

sumerman commented Sep 7, 2023

Adding the error as a type parameter doesn't seem like it would be particularly controversial / complex.

👍🏼 I'll send it as another PR

I am somewhat curious what you are intending to use this for though?

We have an AsyncFileReader implementation that, firstly relies on in-process metadata caching, secondly manages its own tokio runtime (we have to be careful with the runtime as we run inside PostgreSQL). So, we can either sidestep object_store or we would have to build both an object_store impl that is compliant with our runtime needs and AsyncFileReader that can use the metadata cache, but delegates the rest to the object_store.

@tustvold tustvold merged commit 6fdbc26 into apache:master Sep 7, 2023
@tustvold
Copy link
Contributor

tustvold commented Sep 7, 2023

Makes sense, I do hope to eventually flesh out a first-party caching story, but I don't have anything to show for it as this time 😅

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.

2 participants