-
Notifications
You must be signed in to change notification settings - Fork 817
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 ObjectStore::list_with_offset (#3970) #3973
Conversation
prefix: Option<&Path>, | ||
offset: &Path, | ||
) -> Result<BoxStream<'_, Result<ObjectMeta>>> { | ||
let offset = offset.clone(); |
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.
It is possible to construct the lifetimes to avoid this clone, but it seems unnecessary given we're likely performing a network call here, the overhead of a string clone is not going to be relevant
@@ -371,11 +371,33 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static { | |||
/// | |||
/// Prefixes are evaluated on a path segment basis, i.e. `foo/bar/` is a prefix of `foo/bar/x` but not of | |||
/// `foo/bar_baz/x`. | |||
/// | |||
/// Note: the order of returned [`ObjectMeta`] is not guaranteed |
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 admit I totally forgot about this detail. Is this an artifact of the requests not being in a particular order? or that local filesystems make no guarantees about order? Or that the order isn't guaranteed to be consistent between object stores?
It does seems like S3 and GCS (and even Azure) return in a particular order.
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.
We may be able to guarantee it in that case
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 had a brief play, this is hard to implement on a filesystem as lexicographic sorting doesn't group "directories" together. Consider the case of a a/b.file
and a/a/b.file
, a true lexicographic sort would be as presented, but a lexicographic sort of directories would not.
This API looks good to me! I agree that there don't seem to be any other parameters of interest for list operations. |
77e275c adds an initial implementation, although the behaviour of the listing API with escaped paths is a little perplexing... This could be a localstack bug though |
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 looks good. I'd be happy to add GCS as a follow up.
Which issue does this PR close?
Closes #3970
Closes #3975
Rationale for this change
See ticket. Originally I proposed adding a generic
list_opts
method, however, I decided against this because:What changes are included in this PR?
Are there any user-facing changes?