cycle-safe root_stale_causes #19298
cycle-safe root_stale_causes #19298
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| else: | ||
| for child in cause.children: | ||
| yield from self._gather_leaves(child, level=level + 1) | ||
| candidates = self._get_stale_causes(key=key) |
There was a problem hiding this comment.
an alternative path that i couldn't quite reason through would be to prevent the cycles from being created in this StaleCause "tree"
| for child in cause.children: | ||
| yield from self._gather_leaves(child, level=level + 1) | ||
| candidates = self._get_stale_causes(key=key) | ||
| visited: Set[str] = set() |
There was a problem hiding this comment.
cant hash StaleCause that have children
making the children list a tuple instead just leads to hash entering the same recursion nightmare
| root_causes.append(cause) | ||
| else: | ||
| next_candidates.extend(cause.children) | ||
| visited.add(cause.sort_key) # maybe replace with better key |
There was a problem hiding this comment.
Somewhat surprised that using this sort_key as the unique identifier here isn't causing any problems -- I'd expect there to be cases where there are duplicates here (i.e. a parent's code version AND data version have both updated), and in that case it looks like this code would only capture one of those. easy fix would just be to use a tuple that contains the reason/cause category properties in there as well I think, but maybe I'm missing something
There was a problem hiding this comment.
Ya thats what my comment was getting at. I was curious if I would get any test failures. Can definitely replace the key set up like you are suggesting.
alangenfeld
force-pushed
the
al/01-18-cycle-safe_root_stale_causes
branch
from
c000343 to
c6cf6f2
Compare
alangenfeld
force-pushed
the
al/01-18-cycle-safe_root_stale_causes
branch
from
c6cf6f2 to
818e22a
Compare
similar to #18516, update the root cause calculation to be safe in the event a cycle happens ## How I Tested These Changes was able to verify the fix against a complex standing deployment, have not been able to distill to minimal repro
similar to #18516, update the root cause calculation to be safe in the event a cycle happens ## How I Tested These Changes was able to verify the fix against a complex standing deployment, have not been able to distill to minimal repro
similar to #18516, update the root cause calculation to be safe in the event a cycle happens
How I Tested These Changes
was able to verify the fix against a complex standing deployment, have not been able to distill to minimal repro