search

apache / iceberg-go

Apache Iceberg - Go
https://iceberg.apache.org/
Apache License 2.0
143 stars 35 forks source link

objstore: Replace IO interface with objstore.Bucket #66

Closed thorfour closed 7 months ago

thorfour commented 9 months ago

io.LoadFS now returns an objstore.Bucket interface for interacting with storage backends.

As mentioned in #60 it may be more pragmatic to instead of implementing the io.IO interface for all the potential backends that need to be supported that we replace that interface with the github.com/thanos-io/objstore Bucket interface.

The Bucket interface appears to be a super set of the functionality required by the previous io.IO interface and it allows this project to leverage the many providers that are already supported by the objstore library.

This PR took a stab at what it would take to replace that interface right now. Additional cleanup of comments and variable names is necessary if this is deemed an acceptable path forward.

zeroshade commented 8 months ago

This seems pretty interesting to me, not sure how I feel about it yet. But that said, I wouldn't want LoadFS to return an objstore.Bucket because that would tie us to them as an external facing dependency. I'd still want what we expose there to be an interface defined here for two reasons:

  1. It makes it easier in the future to extend and add customizable schemes / file io implementations etc.
  2. We can swap out the underlying implementation in the future if necessary without breaking any consumers
thorfour commented 8 months ago

This seems pretty interesting to me, not sure how I feel about it yet. But that said, I wouldn't want LoadFS to return an objstore.Bucket because that would tie us to them as an external facing dependency. I'd still want what we expose there to be an interface defined here for two reasons:

  1. It makes it easier in the future to extend and add customizable schemes / file io implementations etc.
  2. We can swap out the underlying implementation in the future if necessary without breaking any consumers

I think that's totally reasonable. I suppose that could simply be accomplished by just defining the bucket interface here and then using the objstore definitions that implement it.

Let me know if this is something you'd actually want to merge. Would be happy to cleanup the PR as necessary

thorfour commented 7 months ago

Seems like this repo is relatively inactive. So I'm going to close this for now, and continue in our fork. Thanks!

zeroshade commented 7 months ago

That's my fault for not responding here, it's been a busy month. My apologies.

I would absolutely be willing to approve a cleaned up PR.