Feat/no random access storage in tests#1232
Conversation
RangerMauve
requested review from
achou11 and
gmaclennan
and removed request for
gmaclennan
achou11
left a comment
achou11
left a comment
There was a problem hiding this comment.
More of an initial glance than a deep review
Haven't wrapped my head around lower level changes making use of ReadyResource (hoping Gregor can provide some eyes on that). Changes to the tests seem to make sense to me though!
test-e2e/manager-basic.js
Outdated
|
|
||
| test('Consistent storage folders', async (t) => { | ||
| // TODO: Figure kout consistant storage with rocksdb | ||
| test.skip('Consistent storage folders', async (t) => { |
There was a problem hiding this comment.
This test only made sense with random-access-storage. This won't be relevant once we migrate to rocksdb so I'm adding a TODO to revisit it when we get to the hypercore migration for it
There was a problem hiding this comment.
This is an important enough test that I think it's important to have something prior to migration - if the folders are not consistent, persistence is lost. This should be possible to implement with random-access-file instead of random-access-memory.
src/blob-store/hyperdrive-index.js
Outdated
| async _close() { | ||
| await Promise.all( | ||
| [...this.#hyperdrives.values()].map((drive) => | ||
| drive.ready().then(() => drive.close()) |
There was a problem hiding this comment.
Awaiting drive.ready() is not necessary, drive.close() will await any _open().
src/blob-store/hyperdrive-index.js
Outdated
| this.#writerKey = null | ||
| } | ||
|
|
||
| async _open() { |
There was a problem hiding this comment.
I suggest we keep the setup in the constructor, that way we don't need to handle the case where writer and writerKey are null. Then in _open only
await Promise.all(
[...this.#hyperdrives.values()].map((drive) => drive.ready())
)
That ensures that close() works as expected.
test-e2e/manager-basic.js
Outdated
|
|
||
| test('Consistent storage folders', async (t) => { | ||
| // TODO: Figure kout consistant storage with rocksdb | ||
| test.skip('Consistent storage folders', async (t) => { |
There was a problem hiding this comment.
This is an important enough test that I think it's important to have something prior to migration - if the folders are not consistent, persistence is lost. This should be possible to implement with random-access-file instead of random-access-memory.
gmaclennan
left a comment
gmaclennan
left a comment
There was a problem hiding this comment.
Sorry it took me a while to fully review this. I think wrapping initialization code in _open() and adding a requirement to await .ready() for classes that use Ready Resource opens up new possibilities for race conditions in the future and is not necessary - the main issue with the test errors were related to closing not opening. I've created a PR #1245 that reverts the async initialization code but keeps the close code. The tests still pass (fingers crossed CI does too!) with random-access-file. TL;DR this keeps the parts of this PR that solve the issue with async storage, and removes the parts which in my view just create additional race-condition opportunities.
* chore: auto-open resources * chore: increase CI timeout
Co-authored-by: Andrew Chou <andrewchou@fastmail.com>
3 participants
Closes #1208