Add omdb command to display db_metadata_nexus_state#9034
Conversation
smklein
requested review from
davepacheco and
jgallagher
and removed request for
davepacheco
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
LGTM, just a handful of nitpicks
| .with_context(|| format!("loading project {project_id}")) | ||
| } | ||
|
|
||
| // DB Metadata |
There was a problem hiding this comment.
Nit - I think we've started to create submodules of db.rs for new functionality, since this file has gotten pretty beefy. Might be worth moving this into its own module?
| get_db_metadata_nexus_rows(opctx, datastore, ¤t_target_blueprint) | ||
| .await?; | ||
| let table = tabled::Table::new(rows) | ||
| .with(tabled::settings::Style::psql()) |
There was a problem hiding this comment.
Looks like we mostly use Style::empty(), with just a handful of psql()s. Should we stick with one? (Maybe we should have a helper for "an omdb Table" that handles both style and padding?)
There was a problem hiding this comment.
$ rg "Style::psql" | wc -l
16
$ rg "Style::empty" | wc -l
85I think there's a pretty sizeable gap here, independent of this PR. I really don't care which we pick (though you're right, empty seems more popular), I just happened to copy the format of something nearby.
Maybe we converge on one style outside this PR, to migrate everything at once?
| Target Blueprint ......<REDACTED_BLUEPRINT_ID>....... @ nexus_generation: 1 | ||
| ID |LAST_DRAINED_BLUEPRINT |STATE |TRANSITIONING_TO | ||
| -------------------------------------+-----------------------+-------+----------------- | ||
| ..........<REDACTED_UUID>........... |n/a |active | |
There was a problem hiding this comment.
It seems a little weird to have n/a in one column and a blank in the other. Would a "transitioning to" of either n/a or even repeat active be clearer, maybe?
There was a problem hiding this comment.
I'll just make both "blank" if nullable. I really want the "Some" values here to be highly visible, which is IMO more obvious if they're blank otherwise.
| pub async fn get_db_metadata_nexus_in_state( | ||
| &self, | ||
| opctx: &OpContext, | ||
| states: &[DbMetadataNexusState], |
There was a problem hiding this comment.
Tiny perf nit - since this function really wants an owned Vec<DbMetadataNexusState>, I think I'd take that as an argument. As written, this method always allocates a new vec. If we change it to accept one, the caller might have to allocate one (e.g., if they themselves only have a slice), but there may also be cases where the caller already has a Vec that they can move here, which would save an allocation.
| } | ||
|
|
||
| /// Iterate over all Nexus zones that match the provided filter. | ||
| pub fn all_nexus_zones<F>( |
There was a problem hiding this comment.
I think this already landed on main?
There was a problem hiding this comment.
I think this got cleared in the merge: be07778
The viewing part of #9008
2 participants
The viewing part of #9008