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

Allow to set SQLiteYStore's database path and document time-to-live #66

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

davidbrochart
Copy link
Collaborator

@davidbrochart davidbrochart commented Dec 1, 2022

This PR allows to set the database path and the document time-to-live parameters of an SQLiteYStore. The previous way of doing it was to pass a YStore class, so this PR is just a way to make it easier to parameterize an SQLiteYStore. Here is a configuration example at the CLI:

jupyter lab --SQLiteYStore.db_path='path/to/my.db' --SQLiteYStore.document_ttl=86400 --collaborative

But more importantly, the default document time-to-live of an SQLiteYStore is now None (instead of 24 hours), which means that the documents' history will never be cleared. I think it is safer to do so, considering the issues that we are still trying to figure out.

cc @ellisonbg @dlqqq

@davidbrochart davidbrochart added bug Something isn't working enhancement New feature or request labels Dec 1, 2022
@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Base: 0.00% // Head: 0.00% // No change to project coverage 👍

Coverage data is based on head (a9e9ec6) compared to base (a85713f).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #66   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          3       3           
  Lines        274     290   +16     
=====================================
- Misses       274     290   +16     
Impacted Files Coverage Δ
jupyter_server_ydoc/app.py 0.00% <0.00%> (ø)
jupyter_server_ydoc/handlers.py 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dlqqq
Copy link
Member

dlqqq commented Dec 1, 2022

Small nit, is it possible to hide the CodeCov warnings in the review UI? It's really noisy and makes review more difficult.

jupyter_server_ydoc/app.py Outdated Show resolved Hide resolved
jupyter_server_ydoc/app.py Outdated Show resolved Hide resolved
jupyter_server_ydoc/handlers.py Outdated Show resolved Hide resolved
jupyter_server_ydoc/handlers.py Show resolved Hide resolved
@davidbrochart
Copy link
Collaborator Author

Small nit, is it possible to hide the CodeCov warnings in the review UI? It's really noisy and makes review more difficult.

Indeed, it's very annoying. Maybe @blink1073 knows?

@blink1073
Copy link
Contributor

I think this is the setting you'd put in codecov.yaml: https://docs.codecov.com/docs/codecovyml-reference#github_checks-github-users-only

@davidbrochart
Copy link
Collaborator Author

Thanks, actually it's easy to hide annotations in the file view.

Copy link
Member

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

Looks good, you can resolve my other conversations.

dlqqq and others added 4 commits December 6, 2022 10:27
* make configurable traits local to JupyterSQLiteYStore

* Lint

* Set YStore config on class before instantiation

Co-authored-by: David Brochart <[email protected]>
@davidbrochart davidbrochart merged commit c7da143 into jupyterlab:main Dec 6, 2022
@davidbrochart davidbrochart deleted the ystore branch December 6, 2022 12:03
@dlqqq
Copy link
Member

dlqqq commented Dec 6, 2022

@davidbrochart This looks good. Can traits be modified at runtime? The implementation we worked on in davidbrochart#1 doesn't allow this, but this might be OK if we document it in the future.

@davidbrochart
Copy link
Collaborator Author

Can traits be modified at runtime?

Traits can, but it doesn't make sense for config traits. And changing YStore types at runtime makes even less sense.

hbcarlos pushed a commit to hbcarlos/jupyter-collaboration that referenced this pull request Jan 29, 2023
…upyterlab#66)

* Allow to set SQLiteYStore's database path and document time-to-live

* No need for sqlite_ystore_factory()

* make configurable traits local to JupyterSQLiteYStore (#1)

* make configurable traits local to JupyterSQLiteYStore

* Lint

* Set YStore config on class before instantiation

Co-authored-by: David Brochart <[email protected]>

Co-authored-by: david qiu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants