CLI: Improve error messages when coordinator is not running #254
Conversation
| let mut session = match connect_to_coordinator() { | ||
| Ok(session) => session, | ||
| Err(_) => { | ||
| start_coordinator(coordinator).wrap_err("failed to start dora-coordinator")?; |
There was a problem hiding this comment.
I'm not a fan of spawning a coordinator if we failed to connect to the coordinator. What if the dora-coordinator should be on a different machine?
I think its ok to fail the request if the coordinator is not reachable.
There was a problem hiding this comment.
We only do this for the dora up command, which I imagine will be mostly used for local testing.
There was a problem hiding this comment.
Right, I thought it was on all connections sorry!
I still feel like if there is a need to run up two times to get the dora-coordinator up, it should not be a feature and there should be an investigation. What if the first spawning worked but we did not reach it and end up somehow with two coordinator.
There was a problem hiding this comment.
Are you talking about the "failed to start dora-coordinator" error?
There was a problem hiding this comment.
I misread the file, the first time, and I was spooked by the spawn on Err(_), but I misunderstood it at a failure of spawning a first time.
But, I still think that we should not start_coordinator on failing to connect to the coordinator. Let say there is an internet connection issue and we fail to connect to the coordinator. That will makes us try to spawn a new coordinator.
I think that the design pattern Err(_) => Spawn might create even more error. I would prefer to try spawning the coordinator without trying to connect to the coordinator and raise an error on the port being already taken or an error of the kind.
This seems to be more natural and more common when spawning servers.
But open for discussion
There was a problem hiding this comment.
I would prefer to try spawning the coordinator without trying to connect to the coordinator and raise an error on the port being already taken or an error of the kind.
I'm generally fine with this approach as well, but unfortunately this would be a breaking change. Right now, you can run dora up multiple times without errors. So I think it's best to leave the behavior as is for now and revisit this once we're planning version 0.3.0. If you like we can create some sort of tracking issue for the 0.3.0 release so that we don't forget about it.
There was a problem hiding this comment.
It's worth noting that the same applies to the dora check command. Right now, the command assumes a local setup and performs the same logic for checking the coordinator and daemon setup. So we might want to redesign this command too once we have support for distributed deployments (see #256).
There was a problem hiding this comment.
That is true, that it is a breaking change. Make sense to keep it as it is then
We now exit with an error exit code again when no coordinator is running.
This information is not relevant to users and might confuse them.
phil-opp
force-pushed
the
cli-better-errors
branch
from
4e99163 to
5068e14
Compare
This PR improves the reported errors when the CLI fails to connect to dora daemon. Instead of a "connection refused" error, we now output a "could not connect to dora coordinator" error.
Fixes #250