refactor(#2908): resolve a wire id in the graph, not the c0re layer
mara on !2909: "shouldnt node_by_id be part of jobq?" — yes. Resolving a raw value to a `NodeId` is the exact inverse of `NodeId::get`, which already lives in hive-jobq, and it is only a search because the graph owns the counter that makes ids unfabricable. Both halves of that round-trip belong on the same side of the crate boundary. Placing it in `QueueInner` also put it in a layer slated for removal, so the c0re-side helper would have had to move later anyway — and it was private there, leaving any other caller needing the same resolution to write the same `nodes().find_map(…)` by hand. `Graph::resolve_id` replaces it, with a unit test covering the round-trip and the rejection of a value that was never an id. While re-reading the diff for that question: the doc comment added in the previous commit landed *between* `container`'s doc comment and its signature, silently reattaching "The container node of `dag_id`" to the new helper and leaving `container` undocumented. Restored. No behaviour change and no wire change — same search, same call site. Checked with clippy (`--all-targets -D warnings`), `cargo test -p hive-jobq -p hive-c0re` (41 + 322 passed) and `nix fmt`.
This commit is contained in:
parent
eb557ee3c1
commit
0aac20d863
2 changed files with 31 additions and 12 deletions
|
|
@ -345,13 +345,14 @@ impl JobQueue {
|
|||
/// say) is cancelled along with everything else — there is nothing to converge
|
||||
/// when no node ever ran. Only a node that named `Cancelled` survives, and it
|
||||
/// survives because it asked to.
|
||||
///
|
||||
/// `id` names **any node**, not specifically a DAG. Cancelling a group root
|
||||
/// drops that whole group (the cascade is the scheduler's), which is what
|
||||
/// the dashboard's whole-DAG cancel does; cancelling an interior node drops
|
||||
/// just that branch. Nothing here knows about DAGs.
|
||||
pub fn cancel(&self, id: u64) -> bool {
|
||||
let mut inner = self.lock();
|
||||
let Some(node) = inner.node_by_id(id) else {
|
||||
let Some(node) = inner.sched.graph().resolve_id(id) else {
|
||||
return false;
|
||||
};
|
||||
if !inner.sched.cancel_node(node) {
|
||||
|
|
@ -464,17 +465,6 @@ impl QueueInner {
|
|||
|
||||
/// The container node of `dag_id` — the `NodeKind::Dag` root whose id equals
|
||||
/// `dag_id`. `NodeId` is un-fabricable from a raw `u64`, so this is a search.
|
||||
/// Resolve a raw wire `u64` to a graph [`NodeId`], whatever kind of node it
|
||||
/// names. `NodeId` is un-fabricable from a `u64`, so this is a search —
|
||||
/// same cost as [`QueueInner::container`], without asserting the node is a
|
||||
/// DAG container.
|
||||
fn node_by_id(&self, id: u64) -> Option<NodeId> {
|
||||
self.sched
|
||||
.graph()
|
||||
.nodes()
|
||||
.find_map(|n| (n.id.get() == id).then_some(n.id))
|
||||
}
|
||||
|
||||
fn container(&self, dag_id: u64) -> Option<NodeId> {
|
||||
self.sched.graph().nodes().find_map(|n| {
|
||||
(n.parent.is_none()
|
||||
|
|
|
|||
|
|
@ -461,6 +461,21 @@ impl<N, R> Graph<N, R> {
|
|||
self.nodes.iter().find(|n| n.id == id)
|
||||
}
|
||||
|
||||
/// Resolve a raw value back to the opaque [`NodeId`] it names — the inverse
|
||||
/// of [`NodeId::get`], and the only way to perform that direction. A caller
|
||||
/// holding a value that crossed a wire cannot fabricate an id from it (that
|
||||
/// impossibility is the point of the type), so it has to be matched against
|
||||
/// the graph, which is what makes this a search rather than a cast.
|
||||
///
|
||||
/// `None` when no node carries that value, which covers both a value that
|
||||
/// was never an id and one whose node has since been reaped.
|
||||
#[must_use]
|
||||
pub fn resolve_id(&self, raw: u64) -> Option<NodeId> {
|
||||
self.nodes
|
||||
.iter()
|
||||
.find_map(|n| (n.id.0 == raw).then_some(n.id))
|
||||
}
|
||||
|
||||
/// Every node in the graph, in insertion order. The scheduler iterates
|
||||
/// this to find runnable pending nodes.
|
||||
pub fn nodes(&self) -> impl Iterator<Item = &Node<N, R>> {
|
||||
|
|
@ -729,6 +744,20 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_id_inverts_get_and_rejects_a_value_that_was_never_an_id() {
|
||||
let mut g: Graph<&str, String> = Graph::new();
|
||||
let a = g.insert("a", vec![], None).unwrap();
|
||||
let b = g.insert("b", vec![], None).unwrap();
|
||||
// Round-trips every id the graph handed out: this is the only way back
|
||||
// from a raw value, since NodeId can't be constructed from one.
|
||||
assert_eq!(g.resolve_id(a.get()), Some(a));
|
||||
assert_eq!(g.resolve_id(b.get()), Some(b));
|
||||
// A value that was never an id resolves to nothing, so a caller can't
|
||||
// reach a node by guessing a number off the wire.
|
||||
assert_eq!(g.resolve_id(u64::MAX), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn state_terminality() {
|
||||
assert!(State::Done.is_terminal());
|
||||
|
|
|
|||
Loading…
Reference in a new issue