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

fix: ensure "local" ObjectStore.list returns ordered elements #26081

Merged
merged 1 commit into from
Feb 28, 2025

Conversation

waynr
Copy link
Contributor

@waynr waynr commented Feb 27, 2025

While initially attempting to reproduce a data corruption issue in enterprise, I encountered unexpected behavior during compactor restarts -- namely, that the compactor would always load the wrong compaction summary. It should just load the latest compaction summary which would show up as the first item in an object store listing if the object store listed items in UTF-8 binary order (ascending).

This is the case when using the S3-backed object store, but not when using the LocalFileSystem.

Indeed, the ObjectStore trait explicitly doesn't guarantee the order of list results.

This PR attempts to address this issue by wrapping the returned LocalFileSystem instances with one that collects the inner implementation's list operation results into a vec and sorts them before streamin them back to the caller.

One downside of this approach is that it will cause a tokio thread to block due to the use of futures::executor::block_on, but I don't know of any better way to collect results from the inner LocalFileSystem.list's returned BoxStream<...> value.

The reason that's necessary is that ObjectStore.list isn't an async function so we can't await it without invoking some kind of async executor.

I went with this approach in spite of this downside because...

  • The problem seems to be specific to the LocalFileSystem object object_store.
  • Implementing the collection and sorting outside of the ObjectStore API would make it easy to miss a case where ordering of the object store list results is important and leave a bug for future developers to come across.
  • The --object-store=file implementation is meant for local dev testing, not production -- I think it's fine to be more relaxed about blocking a tokio thread in this restricted scope.

Copy link
Contributor

@hiltontj hiltontj left a comment

Choose a reason for hiding this comment

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

LGTM - the blocking operation should be okay in my opinion. In addition to your notes:

  • file store could be used in production, but it would only be in a single node setup;
  • LIST operations are usually only used on startup when blocking operations probably aren't as costly, and are not used during regular operation of the process.

@waynr waynr merged commit a84c4a9 into main Feb 28, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants