search

src-d / engine-deprecated

[DISCONTINUED] Go to https://github.com/src-d/sourced-ce/
https://docs.sourced.tech/engine
Apache License 2.0
217 stars 26 forks source link

Engine state #376

Closed smacker closed 5 years ago

smacker commented 5 years ago

Fix: #372

smacker commented 5 years ago

I'm investigating why CI fails. Tests pass locally.

smacker commented 5 years ago

Found the issue and fixed. Though travis doesn't work today so I can't re-test it

se7entyse7en commented 5 years ago

LGTM, with some comments, but not blocking this PR.

In general, by reading the flow of the operations we do to ensure that the daemon is running, I have the impression that to get from A to B we first pass through C.

For example, I'd expect ensureStarted to first of all check whether the daemon is running and return it. But we first check for the state file, if it doesn't exist we run Start that saves the state (but it shouldn't be necessary if the daemon is already running as the current state file should be up-to-date), then we finally call InfoOrStart that returns the running container. Additionally given that Start just ignores the first return value InfoOrStart, when we call ensureStarted we end up calling docker.Info twice.

Analogously when the state file is present, we decode the state file, and then we ignore it if the daemon is already running.

I'd suggest to open an issue for improving the daemon package.

smacker commented 5 years ago

thanks Marvin. I better fix it in this PR. At least partially.

smacker commented 5 years ago
  1. I have fixed the mentioned problem in the flow. It still can run info more than once in some cases (like we use docker.InfoOrStart in start) but it's a very cheap operation and doesn't introduce logical complexity as previous flow.
  2. Rebased on master
carlosms commented 5 years ago

There is a small change in how ensureStarted works. It is a bit of a corner case, but before you could do srcd stop, cd to another repo dir, run srcd sql "..." and it would query on the new dir. Now it will open the last workdir.

I'm not worried about this specific use case, because you can run init explicitly, and that's how it is documented. But I'm thinking out loud: should differentiate a daemon crash to a stop? I mean deleting the state on stop. Maybe there is some other use case that relies on that.

smacker commented 5 years ago

Could you please explain your concern in more details? I don't really understand it.

How it works on master: init dirA -> Everything ok case: sql query is executed on dirA, no matter did you do cd or not Stop or restart docker or restart OS or some crash: sql query runs on current directory

How it works with this pr: init dirA -> Everything ok case: sql query is executed on dirA, no matter did you do cd or not Stop or restart docker or restart OS or some crash: sql query is executed on dirA, no matter did you do cd or not

To change the directory you need to run init.

smacker commented 5 years ago

Ah. I think I got what you mean. Difference between stop and prune. This PR makes similar to docker. stop in docker doesn't remove container/state. rm/prune does. We may use stop in engine as stop&rm in docker though, I don't have an opinion on it.

carlosms commented 5 years ago

What I mean is that in master these two are equivalent:

cd /tmp/a
srcd init
srcd stop

cd /tmp/b
srcd init
srcd sql "select * from repositories"
# shows repos in /b
cd /tmp/a
srcd init
srcd stop

cd /tmp/b
srcd sql "select * from repositories"
# shows repos in /b

After this PR, in the second sequence of commands srcd sql will restart the daemon with the latest workdir, /tmp/a, and show the list of repos in /a. As I said the specific use case is not so important to me, but still I wanted to discuss it in case there are some other use cases affected that are more important to support.

se7entyse7en commented 5 years ago

Regarding the stop behavior I don't have a strong opinion, I think they're both ok until it is clearly documented.