-
Notifications
You must be signed in to change notification settings - Fork 40
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
Update Oximeter-related DataStore methods for incoming reconfigurator support #6518
Conversation
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.
Looks good, thanks! I have a few questions and small suggestions, but the bulk of the work looks great to me.
use db::schema::oximeter::dsl; | ||
|
||
dsl::oximeter | ||
.select(( |
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.
Does AsExpression
help here? It seems like we might be able to derive this on the ProducerEndpoint
model type, and then use it 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.
I'm not sure! I tried it for a bit and couldn't get it to work; it requires also attaching a #[diesel(sql_type = Foo)]
annotation, and I have no idea what to put in for Foo
(a tuple of the table's columns doesn't work). Do we use AsExpression
on structs with multiple fields anywhere?
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 question, I don't see any such types! I don't think it's a huge deal, it's just frustrating to have to rename all the columns explicitly.
This PR adds two new
DataStore
methods:oximeter_delete
(soft deletes an oximeter instance, which also requires a small migration to add atime_deleted
column)oximeter_reassign_all_producers
(reassigns all metric producers from one oximeter instance to random other instances)and updates existing methods to account for the possibility that an oximeter instance may have been deleted (e.g., don't return deleted instances when listing).
There are no callers of these methods in this PR, but there will be in the upcoming reconfigurator-can-rebalance-oximeter PR.