-
Notifications
You must be signed in to change notification settings - Fork 99
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
feat: dangling node detection for graph.Memory #652
Conversation
Signed-off-by: Xiaoxuan Wang <[email protected]>
Signed-off-by: Xiaoxuan Wang <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #652 +/- ##
==========================================
+ Coverage 75.23% 75.32% +0.08%
==========================================
Files 59 59
Lines 5480 5487 +7
==========================================
+ Hits 4123 4133 +10
+ Misses 1001 999 -2
+ Partials 356 355 -1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Xiaoxuan Wang <[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.
Hi!
Thank you for this PR, I think this is a first good step toward "chain deleting".
I just have one minor comment, otherwise the code looks fine.
Best regards.
key := descriptor.FromOCI(desc) | ||
_, existsInMemory := m.nodes[key] | ||
_, existsInPredecessors := m.predecessors[key] | ||
return existsInMemory && !existsInPredecessors |
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.
At a first glance, I missed the !
.
I would add a comment stating: "a node is dangling if present in memory without any predecessor", e.g.:
A (root) -> B -> C
D -> E
D is dangling here.
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.
Good catch. Added the doc comment. A public function should always has doc comments.
Signed-off-by: Xiaoxuan Wang <[email protected]>
close this PR as its content is contained in #653 |
Part of #472
This PR:
graph.Memory
graph.Memory.Remove()
return dangling successorsThis draft PR is followed by #653, which implements recursive GC on
oci.Store
.