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

Database access refactor #676

Merged
merged 7 commits into from
May 28, 2024
Merged

Conversation

KIRA009
Copy link
Contributor

@KIRA009 KIRA009 commented May 17, 2024

What kind of change does this PR introduce?
This PR differentiates the access levels of the database among the various processes

Summary
This PR differentiates the access levels of the database among the various processes, and introduces two conventions.

  • Processes which only require data from the database, without wanting to write back to it, should use crud.get_new_session(read_only=True)
  • Processes that require write access, must first acquire the write lock, then use the session returned by crud.get_new_session(read_and_write=True) to make changes to the database. Once all operations are complete, the process must release the lock using crud.release_db_lock. By default, any other process trying to acquire the lock will wait for 60 seconds, after which it will return False.
  • The lock in question, is a simple txt file suffixed by .lock, which contains information about the process id of the process which acquired the lock, and when the lock was acquired. When the lock is released, the file is deleted, and any other process can acquire the lock by creating a new file. This will allow multiple disparate processes to share the idea of locking of database writes, given that they follow the convention described above, and is fairly safe from race conditions given that we are unlikely to face high number of write requests at the same time.
    • Ideally, at the start of the python -m openadapt.entrypoint, there should be no lockfile, but just in case, running the entrypoint module, cleans the lock file and starts anew, so special attention needs to be put here.

Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have performed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

How can your code be run and tested?

Other information

Copy link
Contributor

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @KIRA009 ! I left a few comments, happy to chat about any of it if you like.

experiments/imagesimilarity.py Outdated Show resolved Hide resolved
openadapt/app/dashboard/api/recordings.py Outdated Show resolved Hide resolved
openadapt/app/tray.py Outdated Show resolved Hide resolved
openadapt/db/db.py Outdated Show resolved Hide resolved
openadapt/db/crud.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
Copy link
Contributor

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

LGTM!

@abrichr abrichr merged commit 987f6ac into OpenAdaptAI:main May 28, 2024
1 check passed
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

2 participants