Rework clean and iter_objects for topological traversal - #1288
Open
jrray wants to merge 2 commits into
Open
Conversation
jrray
force-pushed
the
clean-in-topological-order
branch
from
October 24, 2025 21:08
b313368 to
74f07c7
Compare
jrray
changed the title
jrray
added
bug
jrray
force-pushed
the
clean-in-topological-order
branch
from
October 24, 2025 21:20
25d9c79 to
50b5ce6
Compare
jrray
marked this pull request as draft
October 24, 2025 21:20
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
jrray
force-pushed
the
clean-in-topological-order
branch
from
October 24, 2025 22:22
50b5ce6 to
b405511
Compare
jrray
marked this pull request as ready for review
October 24, 2025 22:35
jrray
force-pushed
the
clean-in-topological-order
branch
from
October 24, 2025 23:12
b405511 to
ba586aa
Compare
jrray
force-pushed
the
push-volovwxpnxry
branch
2 times, most recently
from
October 25, 2025 18:19
2640233 to
9ef4438
Compare
jrray
force-pushed
the
clean-in-topological-order
branch
from
October 25, 2025 18:19
ba586aa to
9aae863
Compare
To address the problem demonstrated in #1282, clean needs to delete things in a top-level order, and skip cleaning children of anything deemed uncleanable. The plan is to make iter_objects (now iter_items) return more detailed information including what the item's parent(s) are, so that the consumer of the stream can build up a graph of items. Since iter_items can be called over RPC and used as a stream, it isn't practical to compute the entire graph and return it in a single response. But the stream items can be used to build up the graph incrementally. Signed-off-by: J Robert Ray <jrray@jrray.org>
This is only true when there are no concurrent writers! This is the test from #1282 but passes as of the changes in this PR. Signed-off-by: J Robert Ray <jrray@jrray.org>
jrray
force-pushed
the
push-volovwxpnxry
branch
from
October 25, 2025 18:31
9ef4438 to
1c49e90
Compare
jrray
force-pushed
the
clean-in-topological-order
branch
from
October 25, 2025 18:31
9aae863 to
8fecf80
Compare
jrray
commented
Oct 27, 2025
Comment on lines
+669
to
+673
| let mut dfs = Dfs::new(&g, node_idx); | ||
| while let Some(n) = dfs.next(&g) { | ||
| let node = idx_to_node.get(&n).unwrap(); | ||
| attached_nodes.insert(node.digest()); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Note to self, this should also mark these objects as attached in self.attached.
jrray
commented
Oct 27, 2025
Comment on lines
+717
to
721
| let mut dfs = Dfs::new(&g, node_idx); | ||
| while let Some(n) = dfs.next(&g) { | ||
| let node = idx_to_node.get(&n).unwrap(); | ||
| attached_nodes.insert(node.digest()); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Note to self, this should also mark these objects as attached in self.attached.
rydrman
approved these changes
Dec 3, 2025
rydrman
left a comment
rydrman
left a comment
Collaborator
There was a problem hiding this comment.
Just reminding that you have a couple of notes to self on this one, in case you intend to resolve those still
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To address the problem demonstrated in #1282, clean needs to delete things in a top-level order, and skip cleaning children of anything deemed uncleanable.
The plan is to make iter_objects (now iter_items) return more detailed information including what the item's parent(s) are, so that the consumer of the stream can build up a graph of items. Since iter_items can be called over RPC and used as a stream, it isn't practical to compute the entire graph and return it in a single response. But the stream items can be used to build up the graph incrementally.
It's working as a proof of concept (as in all tests pass) but I didn't update the rpc repo type for these changes
and handling payloads is unfinished. I'm surprised it even compiles with the server feature enabled, as I was avoiding turning that feature on until I got the basics working.Edit: I see, object iteration over rpc isn't supported.
spk/crates/spfs/src/server/database.rs
Lines 74 to 81 in 8b75563