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

feat: add method to unlink storage #34

Merged
merged 2 commits into from
Jan 30, 2024
Merged

feat: add method to unlink storage #34

merged 2 commits into from
Jan 30, 2024

Conversation

EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Jan 19, 2024

This adds MultiCoreIndexer.prototype.unlink() which unlinks all the index storage.

Addresses #26. Depends on #35 because it uses ??=.

@EvanHahn EvanHahn force-pushed the unlink branch 2 times, most recently from af8c70f to 4d8c94a Compare January 19, 2024 02:10
@EvanHahn EvanHahn changed the base branch from main to update-node-versions January 19, 2024 02:11
@EvanHahn EvanHahn marked this pull request as ready for review January 19, 2024 02:12
Base automatically changed from update-node-versions to main January 19, 2024 13:41
This adds `MultiCoreIndexer.prototype.unlink()` which unlinks all the
index storage.

Addresses #26.
@EvanHahn EvanHahn linked an issue Jan 22, 2024 that may be closed by this pull request
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

first time i'm seeing ??= in app code - took a second to understand that it short-circuits, which makes me have mixed feelings about its existence 😄

Changes look good overall, but had a small question that I'd consider non-blocking

Comment on lines +115 to +116
this.#storage ??= await this.#createStorage()
await unlinkStorage(this.#storage)
Copy link
Member

Choose a reason for hiding this comment

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

i'm assuming there's an edge case that this accounts for but just in case: would a no-op (or throw) be possible here if the storage isn't initialized? e.g. if (!this.#storage) return

this seems to be performing operations that cancel each other out if the storage isn't already initialized, but maybe it's much more complex than that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be cases where the storage has been created on disk but hasn't been initialized in memory. For example:

  1. I start the app, index a bunch of stuff, and then close it.
  2. Later, I start the app and immediately unlink. this.#storage won't be created. We want to create it so we can unlink it.

My understanding is that creating the storage instance doesn't actually touch any files on disk. For example:

const RandomAccessFile = require("random-access-file")
const fs = require("node:fs")

new RandomAccessFile("my-file.txt")

fs.existsSync("my-file.txt")
// => false

Maybe it's worth adding an explainer comment about this?

Copy link
Member

Choose a reason for hiding this comment

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

ah okay that makes more sense! I was under the impression that this class fully manages the storage on disk but if it's being done somewhere else, your reasoning is clearer. thanks!

@EvanHahn
Copy link
Contributor Author

I'll plan to merge this tomorrow barring objections. (Feel free to merge sooner if you want!)

@EvanHahn EvanHahn merged commit 98a0e25 into main Jan 30, 2024
4 checks passed
@EvanHahn EvanHahn deleted the unlink branch January 30, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method for resetting index state
2 participants