-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add support for object attributes in Iter call #63
base: main
Are you sure you want to change the base?
Conversation
This commit adds support for passing in object attributes to the Iter callback. This PR makes a breaking change to the API since it adds a new parameter to the callback. The option is currently supported for GCS and Filesystem buckets only, but can be extended to other providers on demand. An alternative implementation would be to add a new IterWithAttrs method and implement it only for a subset of providers. This will avoid a breaking change and will make the support of the feature more explicit. The downside is we need a new interface for something that is only an option and that could be supported for all providers in the future. Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change a lot!
return p.bkt.Iter(ctx, pdir, func(s string) error { | ||
return f(strings.TrimPrefix(s, p.prefix+DirDelim)) | ||
return p.bkt.Iter(ctx, pdir, func(s string, _ ObjectAttributes) error { | ||
return f(strings.TrimPrefix(s, p.prefix+DirDelim), EmptyObjectAttributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why wouldn't it be passed through here?
// Entries are passed to function in sorted order. | ||
Iter(ctx context.Context, dir string, f func(string) error, options ...IterOption) error | ||
Iter(ctx context.Context, dir string, f func(name string, attrs ObjectAttributes) error, options ...IterOption) error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're already breaking this signature, I think we should instead make the whole thing pass only the ObjectAttributes
struct and add the Name
field to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense, and would allow us to extend supported attributes in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than breaking the API, could we add a new API?
Yeah I will look into adding a new APi. I am still not sure how to handle cases where a provider does not support returning attributes. |
An idea may be having a function on the objstore client to check if the feature is supported. Then the Iter() contract is that if the feature is supported, attributes will be fetched, otherwise if the feature is not supported it will error out. It's caller responsability to check if the feature is supported by the actual objstore client and take an informed decision about requesting attributes or not. |
@fpetkovski are there plans to revive this pr? :) Loki project is planning to switch to thanos clients. Having this change will reduce the fanout of listing objects along with metadata which now requires Iter call followed by fetching attributes for all objects |
This commit adds support for passing in object attributes to the Iter callback. This PR makes a breaking change to the API since it adds a new parameter to the callback. The option is currently supported for GCS and Filesystem buckets only, but can be extended to other providers on demand.
An alternative implementation would be to add a new IterWithAttrs method and implement it only for a subset of providers. This will avoid a breaking change and will make the support of the feature more explicit. The downside is we need a new interface for something that is only an option and that could be supported for all providers in the future.
Changes
Verification