-
Notifications
You must be signed in to change notification settings - Fork 2
ThreadDatastore refactor - synchronous backends #54
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
Conversation
This reverts commit 55b118c.
…tastore into threadpool-refactor
|
|
||
| type | ||
|
|
||
| ThreadBackendKinds* = enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the open close principle - https://en.wikipedia.org/wiki/Open%E2%80%93closed_principle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but I don't believe it impedes any of the goals of using TieredDs or CacheDs, etc. Those all use composition-over-inheritance which meets the needs and goals here. Plus there's plenty of (IMHO fair) criticism of the open-close principle at least when using inheritance. The linked article points out "Identify points of predicted variation and create a stable interface around them" which we have at the Datastore object level and it works well.
That said I don't necessarily like the case-type in this instance. So I made a generic version here https://github.com/codex-storage/nim-datastore/pull/54/files . If generic methods disappeared, it'd be easy create generic async procs to do the work and call them from the methods. You'd need a specific thread implementation for each ThreadedSqliteDs* = ref object of Datastore: ds: SqliteDs[...], etc, but at that point we could probably remove non-threaded SQLiteDatastore.
In general it makes sense to be careful about what actions are run on the threadpools. libuv for example only has a set enumerate list of actions it supports for file operations. Though it does add a uv_queue_work but we have nim-taskpools for those needs. We could do similar and have an enumerate set of io-thread-pool commands, which is sort of what ThreadBackendKinds is doing here.
|
Closing in favor of #55 with FsDs |
Refactor sqliteds into a synchronous backend. The ThreadDatastore was then refactored to use the synchronous sqlite backend. The ThreadDatastore is based on #48 but with simplifications due to not needing to handle Futures on the backend.
The FsDs should now be pretty easy to refactor by re-using the design for SqliteDs.
Other changes:
string | KeyIdand data asseq[byte] | DataBufferSome todos:
SQLiteDatastorebut it's query iterator would need to be wrapped into a QueryIter