Implement YStore versioning #48
Conversation
davidbrochart
force-pushed
the
version
branch
2 times, most recently
from
cd02839 to
8ea0486
Compare
ypy_websocket/ystore.py
Outdated
| version = int(await f.readline()) | ||
| if version != self.version: | ||
| raise YDocVersionMismatch( | ||
| f"YStore version mismatch: got '{version}' in file, but supported is '{self.version}'" |
There was a problem hiding this comment.
Should we automatically remove the store or add a sentence like: You can remove file '...' to fix this error - WARNING it will erase the documents edition history.
There was a problem hiding this comment.
I think removing the DB is too much, as the user could decide to still use it with another ypy-websocket version.
But it's true that it would be nice to not error out, as most of the time users want things to just work. What about moving the out-of-date DB to a new name, like .jupyter_ystore(1).db, .jupyter_ystore(2).db, etc.?
There was a problem hiding this comment.
I am not a fan of renaming for two reasons:
- It is gonna fill the user disk with files that are useless for most users
- It does not solve the trouble of an user that use another ypy-websocket as it may even enter a recursive loop of database renaming.
That said what about making the version part of the naming?
Let's ping folks to get idea @afshin @ellisonbg
There was a problem hiding this comment.
- It is gonna fill the user disk with files that are useless for most users
We could move it to the OS's temporary directory.
- it may even enter a recursive loop of database renaming.
I don't get that part, why would it?
There was a problem hiding this comment.
| It is gonna fill the user disk with files that are useless for most users
We could move it to the OS's temporary directory.
You will get into troubles for user spawning multiple Jupyter servers from multiple folders (a la VS Code).
it may even enter a recursive loop of database renaming.
if the user has two environments (envA and envB) that uses two different versions of the store, the user could end up in the following scenario:
- Work with envA -> .jupyter_ystore.db is created
- Stop envA
- Work with envB -> .jupyter_ystore.db is renamed .juypter_ystore(1).db and new db is created
- Stop envB
- Work back with envA -> .jupyter_ystore.db is renamed and a new deb is created
- ...
There was a problem hiding this comment.
Thanks for the explanation, then including the version in the name would work indeed.
There was a problem hiding this comment.
Thinking about it, making the version part of the file name is also problematic, as we now have to ensure that the version in the file name and the version in the database match.
ypy_websocket/ystore.py
Outdated
| cursor = await db.execute("pragma user_version") | ||
| version = (await cursor.fetchone())[0] | ||
| if version != self.version: | ||
| raise YDocVersionMismatch( |
ypy_websocket/ystore.py
Outdated
| @@ -160,7 +195,7 @@ async def read(self) -> AsyncIterator[Tuple[bytes, bytes]]: # type: ignore | |||
| raise YDocNotFound | |||
|
|
|||
| async def write(self, data: bytes) -> None: | |||
| await self.db_created.wait() | |||
| await self.db_initialized | |||
There was a problem hiding this comment.
Not related but should we invert line 198 and line 199. The rational behind it is that the metadata is generating a timestamp. If the initialization takes some time, the timestamp will shift a bit. It may be more interesting to get the timestamp matching the write execution request.
There was a problem hiding this comment.
The metadata that we get from get_metadata() is generic, even though it is indeed a timestamp in jupyter-server-ydoc currently.
I think you are referring to the timestamp column. But anyway, that's a good point and I think we should move both before waiting for the DB to be ready, as they are independent.
davidbrochart
force-pushed
the
version
branch
9 times, most recently
from
39b9fe0 to
b1cae10
Compare
davidbrochart
force-pushed
the
version
branch
from
b1cae10 to
06fdff2
Compare
|
@fcollonval I'm going to merge this. |
Closes #43.