Only one instance for each (host+port, client, user) #21
Conversation
|
What happens if the program is killed by SIGTERM and the Doesn't look like we inform the user where the lock file is in case it prevents the program from starting. |
|
On linux it still works, the lock is released by the kernel. I have not tested on windows, but should work since it tries to remove the lock file and the lock on the file should be released by the kernel when it is exiting. Also just relized that you currently cannot run both testing and non-testing at the same time, will fix that. |
|
I think the lock file should probably be in the cache dir instead of the data dir, or what do you think? I think the testing vs non-testing should really depend on the |
|
Regarding if it should be in the cache dir instead of data dir I agree, will fix. Regarding testing vs non-testing i currently disagree. Since host+port is fetched from the config file anyway there is little point in doing it like that. If for example you would run the clients as another user the lock files would be in another directory also so that doesn't become an issue. If you would want to report to two servers it would be better to simply utilize aw-sync for that once that's working. |
|
Strong disagree on removing the ability to send data to multiple hosts from a client. Just to clarify: Testing will behave the same (pick a special host and port in the config) unless specified. The lockfile name should include About sync: The server instances listening to different hosts and ports might not want to sync. Example: one watcher sends to some data collection service (automatic sharing/status/analysis services, studies, etc.) and the other to a local server as the users own copy.
The user might want to "go live" with their data using some service, adding a server sync would increase latency significantly making the reporting happen after 2 syncs. I don't think we should limit We should offer host and port keyword parameters on the ActivityWatchClient constructor. This is needed if we want to be able to specify host and port for a single run using cli arguments. So applications managing the watcher can set some settings temporarily without having to deal with the config file (by overriding settings set there). But that's not needed in this PR. We'll do that when the time comes. |
ErikBjare
changed the title
|
But we can merge this for now if you want. Are you done? |
| from .config import load_config | ||
|
|
||
| from .client import ActivityWatchClient | ||
| from .client import ActivityWatchClient # noqa |
There was a problem hiding this comment.
Removed the "Import unused" from landscape.io
…d client constructor parameter for server host+port
Fair enough. Your example was rather exotic but valid and the host+port solution is more flexible so let's do it that way. The only issue now would be if someone tried to connect to "localhost:5600" and another client tries at "127.0.0.1:5600" the lock wouldn't work. |
|
I'll admit it might be a bit exotic. But imo it's such a large gain for such a small effort that it cannot be ignored. |
aw_client/client.py
Outdated
| self.server_host = server_host if server_host != None else client_config[configsection]["hostname"] | ||
| self.server_port = server_port if server_port != None else client_config[configsection]["port"] | ||
|
|
||
| self.instance = SingleInstance("{}@{}:{}".format(self.client_name, self.server_host, self.server_port)) |
There was a problem hiding this comment.
I don't think that string will be a valid name for a Windows file. Just use dashes or something in place of @ and :.
aw_client/client.py
Outdated
| self.server_host = server_host if server_host != None else client_config[configsection]["hostname"] | ||
| self.server_port = server_port if server_port != None else client_config[configsection]["port"] | ||
|
|
||
| self.instance = SingleInstance("{}@{}:{}".format(self.client_name, self.server_host, self.server_port)) |
ghost
assigned
Have not tested on windows
Also updated test_client.py for the new aw-core versions
Doesn't fix ActivityWatch/aw-qt#19, but at least makes it less severe