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

Migrate database to LevelDB. #810

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

kanishk98
Copy link
Collaborator

@kanishk98 kanishk98 commented Aug 16, 2019


What's this PR do?

Changes database module used for maintaining settings.json to leveldb. Uses a node wrapper around it.

Any background context you want to provide?

We need to move database operations to one process to avoid app crashes (which used to happen earlier). LevelDB also uses locking and can therefore handle corner cases better than node-json-db.

You have tested this PR on:

  • Windows
  • Linux/Ubuntu
  • macOS

@kanishk98
Copy link
Collaborator Author

saved settings don't seem to reflect when the app loads again

Ah, silly mistake on my part - I was moving around DataStore methods and forgot to run the loadSettings() method on app load.
I'll fix this asap.

app/main/index.ts Outdated Show resolved Hide resolved
@kanishk98 kanishk98 force-pushed the level-db branch 2 times, most recently from dab6948 to a9d19f9 Compare August 17, 2019 08:47
@kanishk98 kanishk98 marked this pull request as ready for review August 18, 2019 16:35
@kanishk98 kanishk98 changed the title [WIP] Migrate database to LevelDB. Migrate database to LevelDB. Aug 18, 2019
@kanishk98
Copy link
Collaborator Author

I think this PR's ready for review now. @priyank-p and @akashnimare please have a look at this whenever you find time. :)

@zulipbot
Copy link
Member

Heads up @kanishk98, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@manavmehta
Copy link
Collaborator

@akashnimare Are there only merge conflicts or there's something else as @kanishk98 says it's ready for review.
Should I make a new PR based on this one ?

@kanishk98
Copy link
Collaborator Author

Hey @manavmehta, great to see some new faces around here!
I'm a little free this weekend, so I'd be happy to rebase against a fresh fetch of master or I can walk you through any help you might need. @akashnimare let us know what works best for you.

@kanishk98
Copy link
Collaborator Author

I think this was a fairly involved PR, so if LevelDB still solves our database-level problems, it would be easier and quicker if I were to just update my PR according to master. I'm just trying to be cognizant of GSoC processes at the same time.

@manavmehta
Copy link
Collaborator

@kanishk98 awesome. In case you and @akashnimare feel I should build upon this, I would surely appreciate your help!
Thanks for showing up :)

Base automatically changed from master to main January 22, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants