Conversation
…nsaction handling
cap10morgan
left a comment
cap10morgan
left a comment
There was a problem hiding this comment.
OK this was fun! ;)
I don't think there's any showstoppers in my review, so I'm approving. But I do have a few questions to consider and requests for a few small / nit-picky changes here and there. Feel free to ignore / delay / question any of it!
This is awesome BTW! 🪨
| let db_size; | ||
| try { | ||
| db_size = (await fs.stat(tableObj.primaryStore.env.path)).size; | ||
| db_size = (await fs.stat(tableObj.primaryStore.path ?? tableObj.primaryStore.env.path)).size; |
There was a problem hiding this comment.
Would it make sense to move this into a method?
There was a problem hiding this comment.
Actually, lmdb-js sets path on the database instance, so this could be simplified to:
db_size = (await fs.stat(tableObj.primaryStore.path)).size;
resources/analytics/write.ts
Outdated
| storeMetric(analyticsTable, metric); | ||
| let metric; | ||
| if (firstTable.primaryStore instanceof RocksDatabase) { | ||
| const dbPath = firstTable.primaryStore.store.path; |
There was a problem hiding this comment.
It looks like in schemaDescribe we're using tableObj.primaryStore.path but here it's .primaryStore.store.path. Is one correct and the other not?
Should we have a method that abstracts the Rocks / LMDB difference and centralizes & standardizes this?
There was a problem hiding this comment.
In rocksdb-js, db.store.path is the source of truth. db.path is a getter that returns the store's path. With that said, while store is publicly accessible, it wasn't meant to be public. The correct usage in this case should be firstTable.primaryStore.path.
| dev: { path: dbPath }, | ||
| test: { path: dbPath }, | ||
| test2: { path: dbPath }, | ||
| }); |
There was a problem hiding this comment.
Perhaps not for this PR, but I've had lots of issues with this not being hermetic w/r/t ~/hdb. Sometimes also setting ROOTPATH seems to help, but other times it doesn't. 🤷🏻
There was a problem hiding this comment.
Yeah, this change was intended to help with this. Previously the dev, test, and test2 folders were being created/used from ~/hdb, but now they should be a part of the process specific folder for unit test execution (isolated to each test run).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
3 participants
Adding support for running on our new Rocksdb-js storage system.
The major changes in this PR are: