-
Notifications
You must be signed in to change notification settings - Fork 58
nexus-db-queries: track blueprint table coverage cumulatively #8988
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for taking this one on. I just wanted to give you a heads-up that I probably won't be able to look for a week or two. (If anyone else at Oxide can take a look, go for it!) |
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.
This looks pretty good. I have a few small suggestions, but we don't have to make most of these changes.
self.counts.values().all(|&count| count == 0) | ||
} | ||
|
||
/// Add counts from another BlueprintTableCounts to this one. |
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.
What do you think about having BlueprintTableCounts
contain an integer counter of the number of blueprints we think should be here? new()
would set it to 1, and add()
would add the count from the other
.
Then in ensure_blueprint_fully_populated()
, for the non-exception tables, their counts should match the count in the struct.
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.
That totally make sense, thank you! Will add it shortly.
"bp_pending_mgs_update_sp", | ||
"bp_pending_mgs_update_rot", | ||
"bp_pending_mgs_update_rot_bootloader", | ||
"bp_pending_mgs_update_host_phase_1", |
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.
I guess you removed these because now the exceptions are tables that are not in any of the representative blueprints. (My other suggestion was assuming this was the same semantics as before, that some blueprints might not have them.)
Do you know if it'd be hard to create representative blueprints that exercise the three clickhouse tables? I feel like you had that in an earlier version of the earlier PR?
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.
Yes I remember the Clickhouse config. You're right now that cumulative tracking is implemented the same pattern can be applied to Clickhouse. I think I can apply the same pattern and it shouldn't be difficult 🙂
bd8bde9
to
8bb5e6f
Compare
This is a follow up PR to #8667 as suggested by @davepacheco in #8667 (comment)
Instead of verifying table coverage for individual blueprint insertions, this change tracks coverage cumulatively across all tests in
test_representative_blueprint()
.