@generall This is -- in my opinion -- the smallest PR that makes sense and doesn't break build. It might still be too big, but it doesn't include the S3 code, so that's one less thing to test and review.

Thank you for the review @generall!

I think overall it is moving into the right direction, however I think it might benefit from making SnapshotManager less global and more independent entity. Here is how I can see that:

• SnapshotManager doesn't really have any internal state. It can be easily created from the configuration (even with s3 support enabled).

All relevant structures in the code (ToC, Collection, Shard) have access to storage configuration, so it should be easy to build SnapshotManager on demand.

Great catch! I didn't realize that but I totally agree.

• Current SnapshotManager knows too much about the whole project structure. E.g. it knows about the concept of shard_ids and collections. At the same time Collection should know about SnapshotManager which either create circular dependency or forces us to drop proper types at compile time (see the commend about u32 as shard is). I propose to make SnapshotManager more local, let it only work with single given "directory" and abstract it away from differenciation of the snapshot types.

I think we can do it even better: in the S3 code to handle SnapshotFile::OutOfPlace correctly I added some code that determines if the path should point to S3 or not. All it did is check if the provided path is inside the configured snapshot path (or a subdirectory). I think we can extend this to all operations, remove SnapshotFile completely, and rely on paths entirely again, which would simplify things and reduce SnapshotManager's dependency on the current structure. Then the S3-specific logic would only kick in if S3 is enabled and the path should point to S3.

I'll make the changes this afternoon.

@generall Made the following changes:

• SnapshotFile has been deleted.
• SnapshotManager has been de-globalized, instead it is instantiated on demand in many different places.
• SnapshotManager is now scoped to a single directory with SnapshotManager::scope. It is unaware of the project structure, other code scopes it to certain directories to achieve certain functionality. (for example, to list all collection snapshots: toc.snapshot_manager().scope("my_collection/").do_list_snapshots().await?)
• SnapshotManager functions that refer to exact snapshots (e.g. do_delete_snapshot) take an impl AsRef<Path> as the "snapshot name". This allows for the normal behaviour if a relative path like test_snapshot.snapshot is given, but also OutOfPlace behaviour is supported if an absolute path like /path/to/my/snapshot.snapshot is given. This allows for versatility while still centralizing snapshot access code. This can also be easily adapted to support S3.

I didn't have time to run any integration tests, so I'd appreciate a CI run. :)

Fixed the failing tests.

Hey @mogery, unfortunately the PR still have too big impact on the internals, which makes it very hard too review and validate.

There are many places where snapshot_manager is introduces as a new function argument, even though it doesn't provide any additional context to the function.

I propose to remove all of those places and try to keep PR at least within 1000 lines of code (ideally within 500 lines)

Closing in favor of #4150