-
Notifications
You must be signed in to change notification settings - Fork 663
[teams 1/5] Reset database #3611
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
Conversation
So far, the `--clear-database` option to `publish` has simply dropped and then re-created the database (if it did exist). This will no longer work when databases can have "children": because dropping and re-creating is not atomic, children would either become orphans, or be dropped as well. To solve this, `reset_database` is introduced as a separate action that: - shuts down all replicas - if a `program_bytes` is supplied, replaces the database's initial program - if a `host_type` is supplied, replaces the database's host type - starts `num_replicas` or the previous number of replicas, which initialize themselves as normal As this could be its own CLI command, the action is provided as its own API endpoint (undocumented). However, since `publish` has no way of knowing whether the database it operates on actually exists, the `publish_database` handler will just invoke the `reset_database` handler if `clear=true` and the database exists, and return its result. This is to avoid starting the transfer of the program in the request body, only to receive a redirect. Some refactoring was necessary to dissect and understand the flow.
bfops
left a comment
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.
It looks like this is an exact copy of #3496. That was approved and merged, but we rolled back because we had some fallout that we have now fixed. So.. this LGTM!
cloutiertyler
left a comment
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 did a deeper review of this one this time around and it looks good to me. It's an elegant solution. Basically just detaches the old replicas from the database and attaches new replicas.
So far, the
--clear-databaseoption topublishhas simply droppedand then re-created the database (if it did exist).
This will no longer work when databases can have "children": because
dropping and re-creating is not atomic, children would either become
orphans, or be dropped as well.
To solve this,
reset_databaseis introduced as a separate action that:program_bytesis supplied, replaces the database's initialprogram
host_typeis supplied, replaces the database's host typenum_replicasor the previous number of replicas, whichinitialize themselves as normal
As this could be its own CLI command, the action is provided as its own
API endpoint (undocumented).
However, since
publishhas no way of knowing whether the database itoperates on actually exists, the
publish_databasehandler will justinvoke the
reset_databasehandler ifclear=trueand the databaseexists, and return its result. This is to avoid starting the transfer of
the program in the request body, only to receive a redirect.
Some refactoring was necessary to dissect and understand the flow.
API and ABI breaking changes
Introduces a new, undocumented API endpoint.
We may want to nest it under
/unstable.Expected complexity level and risk
2
Testing
From the outside, the observed behavior should be as before,
so smoketests should cover it.