[daemon] unified restart, restore and snapshot to grpc::FAILED_PRECON… #3764
Conversation
|
For set, perhaps we could have an |
We can do that, it is just adding a new exception. |
268762f to
12d5f1f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3764 +/- ##
==========================================
- Coverage 88.95% 88.95% -0.01%
==========================================
Files 256 256
Lines 14579 14583 +4
==========================================
+ Hits 12969 12972 +3
- Misses 1610 1611 +1 ☔ View full report in Codecov by Sentry. |
2b19656 to
0032da3
Compare
5c80bf6 to
ff60ac2
Compare
2c3f01e to
3faed2b
Compare
| @@ -72,6 +72,12 @@ class InstanceSettingsException : public SettingsException | |||
| InstanceSettingsException(const std::string& reason, const std::string& instance, const std::string& detail); | |||
| }; | |||
|
|
|||
| class InstanceStateSettingsException : public SettingsException | |||
There was a problem hiding this comment.
Wouldn't it make more sense to derive from InstanceSettingsException (which has the exact same constructor code)?
Conceptually, an InstanceStateSettingsException is-a InstanceSettingsException. OTOH, when we deal with instance-related settings exceptions, we want to include InstanceStateSettingsException too (except when the handling for InstanceStateSettingsException is specific, but then we would place the correponding catch clause first).
Am I missing something? Is there any case where we want to handle InstanceSettingsException but not InstanceStateSettingsException?
There was a problem hiding this comment.
but then we would place the correponding catch clause first).
I think I was thinking that the client code can be order agnostic when I implemented this.
catch (const mp::InvalidSettingException& e)
{
status_promise->set_value(grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, e.what(), ""));
}
catch (const mp::InstanceStateSettingsException& e)
{
status_promise->set_value(grpc::Status(grpc::StatusCode::FAILED_PRECONDITION, e.what(), ""));
}
This would still work. However, maybe it is better to use inheritance because they do conceptually have the is-a relationship.
ff60ac2 to
77b7049
Compare
|
@Sploder12 Sorry that the PR was not rebased so the scope of it got messed up. All your comments are on the clone polishing PR, we can still discuss it for the sake of clarity. |
There was a problem hiding this comment.
@Sploder12 Sorry that the PR was not rebased so the scope of it got messed up. All your comments are on the clone polishing PR, we can still discuss it for the sake of clarity.
No worries, thank you for the clarifying comments! Everything looks good to me!
|
(This is waiting for the base PR to merge first.) |
3faed2b to
9ecef65
Compare
ebe6e34 to
8eb343e
Compare
The base branch was changed.
|
@ricab @Sploder12 , now we can approve the review and get this ready to merge. |
|
Hey @georgeliao, does this need a rebase perhaps? Compilation is failing on win/mac platforms because of an |
|
@ricab will have a look. |
…id state failure.
8eb343e to
19a50a2
Compare
|
@ricab , now it can be merged, I believe. |
…_precondition [daemon] unified restart, restore and snapshot to grpc::FAILED_PRECON…
Daemon::async_wait_for_ready_all->grpc_status_forAbout the client side handler of these changed operations, snapshot, restore and restart all use
standard_failure_handler_forfunction, meaning that there is no distinguishment betweenINVALID_ARGUMENTandFAILED_PRECONDITION. Likewise, set also did not distinguish that via a different way.MULTI-1559
Settle the discussion