Skip to content
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

BLOCKED(discussion ongoing): Default backend db to sqlite #64

Closed
wants to merge 1 commit into from

Conversation

xylix
Copy link
Contributor

@xylix xylix commented Feb 18, 2020

We could notify users that sqlite is the only supported backend going forward, or we could migrate config files via a script.

@ErikBjare Do you have any input on which way we should approach making existing users migrate?

@xylix xylix changed the title WIP: Default backend db to sqlite, fix minor old import WIP: Default backend db to sqlite Feb 18, 2020
@xylix xylix changed the title WIP: Default backend db to sqlite Default backend db to sqlite Feb 18, 2020
@ErikBjare
Copy link
Member

Changing the default should make it automatic. Although I'm unsure if the config will write the defaults on first run and then never be overwritten, in which case migration wouldn't trigger for existing installs.

@xylix
Copy link
Contributor Author

xylix commented Feb 19, 2020

Yes at least I had to manually change my server-testing config to from peewee before the change happened on my local testing config. I made a PR in aw-core, and I'll implement an automatic migration that will always run for peewee users.

@ErikBjare
Copy link
Member

ErikBjare commented Feb 19, 2020

As feared, it looks like aw-core saves the initial defaults as user-set on first run and therefore makes changing an existing default difficult. This needs to be changed. Only explicitly user-set settings should be saved.

If we're doing work on this anyway and need to wipe the .ini files, I suggest we switch over to .toml while we're at it.

@xylix
Copy link
Contributor Author

xylix commented Feb 19, 2020

Makes sense. I'll do it that way.

This PR can be merged then, since the migration will be handed in the aw_core side anyway.

@ErikBjare
Copy link
Member

Also, the .toml change might be overkill/a lot of extra work. You don't have to do that.

Copy link
Member

@johan-bjareholt johan-bjareholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want this to happen because aw-server-rust currently only does automatic migration from the peewee datastore. The sqlite datastore is stable, but it's not worth it to merge it for it to only exist for maybe a few months.

If we merge this and then want to switch to aw-server-rust we have to extend that rust code or let the users manually export/import upon upgrade which we want to avoid.

@ErikBjare
Copy link
Member

ErikBjare commented Feb 19, 2020

Or we could just make the minor changes needed to the aw-server-rust code to automatically import from the sqlite datastore instead?

This PR is a blocker for other work on a settings table in the DB which we pinged you about but you didn't respond to (see the other issue in the main repo).

@xylix
Copy link
Contributor Author

xylix commented Feb 19, 2020

I think the lack of ping might be my fault, because I edited the ping into the body of the post instead of putting it in when I first posted it. Anyway the issue is located at ActivityWatch/activitywatch#348 , check it out @johan-bjareholt

@ErikBjare
Copy link
Member

Also, realistically switching to aw-server-rust won't happen in "a few months" with the current pace of development.

I'm not willing to abandon any significant work on aw-server-python in the meantime, and I'm likely to maintain aw-server-python to somewhat feature parity for quite a while, even after switching the default to aw-server-rust. It's not much work.

Moving aw-server to the sqlite datastore is a major improvement (not to mention dropping the other datastores) and the multiple unsupported datastores (see recent issue about MongoDB in the main repo) is the primary tech debt in the aw-server-python codebase right now.

@johan-bjareholt
Copy link
Member

I think the lack of ping might be my fault, because I edited the ping into the body of the post instead of putting it in when I first posted it. Anyway the issue is located at ActivityWatch/activitywatch#348 , check it out @johan-bjareholt

Not your fault, I've been sick during the weekend and haven't checked my mail. Sorry for that.

Or we could just make the minor changes needed to the aw-server-rust code to automatically import from the sqlite datastore instead?

Then we need two imports in case the user does not migrate from the latest version.

There's also always the risk of the migration not completing properly so the less migrations the better.

This PR is a blocker for other work on a settings table in the DB which we pinged you about but you didn't respond to (see the other issue in the main repo).

It's not a blocker if we finish the migration to aw-server-rust first. Neither is it a blocker if we implement it in peewee. It's a quite large feature and if we are able to only write it once rather than twice we will save a lot of time.

I'm not willing to abandon any significant work on aw-server-python in the meantime, and I'm likely to maintain aw-server-python to somewhat feature parity for quite a while, even after switching the default to aw-server-rust. It's not much work.

It's not much time spending on maintaining it, but having feature parity is and extending the datastore is a perfect example of that. I've been the one making the majority of the feature-parity work in aw-server-rust the past year and it's not been an insignificant amount of work.

Also, realistically switching to aw-server-rust won't happen in "a few months" with the current pace of development.

What's missing? Except for no swagger support I'm not aware of anything.
There's a reason why I just recently pushed it into the main activitywatch repo.
What's needed now is testing on other platforms than Linux and Android.

Also, how come the sqlite datastore suddenly become so important? It's been stable for 2 years and now somehow it's necessary? The same thing can be done in peewee.

@xylix
Copy link
Contributor Author

xylix commented Feb 19, 2020

I guess we should clarify the priorities regarding the rust server.

  • What is the rust-server lacking in extra-features / which of those we can just not support in the first rust-server-default version?
  • Are there any other blockers than testing the rust server out?

Personally I'm interested in helping with the rust-server but I don't have any experience in rust, so I wouldn't be developing as fast there as I am with python / js.

I could work on testing the rust-sever build on linux/android though.

The sqlite didn't become particularly important, it's more about not wanting to implement new features in peewee since it shouldn't be our default database going forward.

@johan-bjareholt
Copy link
Member

What is the rust-server lacking in extra-features / which of those we can just not support in the first rust-server-default version?

As far as I know it's just swagger (the REST UI).
Maybe some third-party watchers are not supported as timestamp parsing is more strict now (based on rfc3339, a subset of iso8601), I've only tested aw-watcher-vim.

Are there any other blockers than testing the rust server out?

Not as far as I know.
I recently fixed every compatibility in the web-ui (the Query Explorer and the Timer was the last thing fixed, the rest has had feature parity for more than half a year, categorization was also broken for a while when that was introduced).

Personally I'm interested in helping with the rust-server but I don't have any experience in rust, so I wouldn't be developing as fast there as I am with python / js.

Yeah I have respect for that.

Depending on what parts of the code it could be either very easy or a bit harder, Rust usually makes complicated things more complicated to solve due to it's safety enforcements but simple things are usually as simple as in most other languages.

Overall though the language switch should be worth it for the project as a whole due to the performance and robustness.
I also expect the majority of changes for activitywatch in the future will be more around the web-ui and clients as the server matures.

I could work on testing the rust-sever build on linux/android though.

I wrote "other" than linux/android. So it's windows and macOS that needs to be tested. I've not tested on macOS at all as I don't have a mac (but unittests and integration tests pass in CI). Windows tests passes as well but it was a while since I tested it manually.

it's also not clear how aw-qt should (or should not in my opinion) handle switching between two different servers.

The sqlite didn't become particularly important, it's more about not wanting to implement new features in peewee since it shouldn't be our default database going forward.

Agreed, my point was that neither of those databases should preferably be used going forward. But apparently me and @ErikBjare have different opinions on how to make the transition of going from aw-server-python to aw-server-rust.

@xylix xylix changed the title Default backend db to sqlite BLOECKED(discussion ongoing): Default backend db to sqlite Feb 19, 2020
@xylix xylix changed the title BLOECKED(discussion ongoing): Default backend db to sqlite BLOCKED(discussion ongoing): Default backend db to sqlite Feb 19, 2020
@xylix
Copy link
Contributor Author

xylix commented Feb 19, 2020

Okay testing on macOS / windows is even easier for me. I'll look into that and wait for input on the other points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants