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

#119: Key-value interface: optional pre-write #120

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

gsvarovsky
Copy link
Contributor

@gsvarovsky gsvarovsky commented Nov 23, 2020

Here is a simple proposal for the key-value interface discussed in #119 .

On the basis that the creator of the quadstore already has access to the raw backend, this proposal does not try to prevent:

  • key collisions with the quad indexes, or
  • abuse of the batch by calling write or clear.

Happy to discuss and add tests/docs when ready.

@jacoscaz
Copy link
Owner

This seems quite good to me already. I'll just leave a couple of nitpick-y comments.

How would you handle reading those custom entries? The Quadstore class already exposes the AbstractLevelDOWN instance through its .db property. Is that enough? I think it would make sense to just leave it at that, without further wrapping or methods. There's a nice symmetry in having direct access to leveldb both at the read and batch stages.

Copy link
Owner

@jacoscaz jacoscaz left a comment

Choose a reason for hiding this comment

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

In addition to my comments:

  • a mention to these hooks in README.md
  • a dedicated test file for these hooks, with one unit test per each hook, preferably using quadstore.db.get() to verify writes

After that I think we should be all set for a new beta.

lib/types/index.ts Outdated Show resolved Hide resolved
lib/quadstore.ts Outdated Show resolved Hide resolved
lib/quadstore.ts Outdated Show resolved Hide resolved
@gsvarovsky
Copy link
Contributor Author

How would you handle reading those custom entries?

I agree – just use the db, or even just read from the AbstractLevelDOWN you supplied in the first place.

@gsvarovsky
Copy link
Contributor Author

I propose that I try this new interface in m-ld, to see if it solves my problem neatly, before we commit to it. It will mean a bit of a delay. Or are you really keen to get the Beta out?

@jacoscaz
Copy link
Owner

I propose that I try this new interface in m-ld, to see if it solves my problem neatly, before we commit to it. It will mean a bit of a delay. Or are you really keen to get the Beta out?

I think this is a really nice and useful PR that complements having direct access to the AbstractLevelDOWN instance, adding to the extendability of quadstore by offering atomic writes of quads + custom data.

I am not in a rush, so it’s really up to you, but I would still like to merge this on its own merit when it’s ready.

@gsvarovsky gsvarovsky marked this pull request as ready for review November 24, 2020 12:54
@jacoscaz jacoscaz merged commit 2cbe497 into jacoscaz:master Nov 24, 2020
@jacoscaz
Copy link
Owner

@gsvarovsky fantastic work - do you require a beta right away?

@gsvarovsky
Copy link
Contributor Author

Great! Not right away. In the next few days would be great, I have one more optimisation in m-ld to work on before I do another pre-release myself.

@jacoscaz
Copy link
Owner

All right, feel free to ping me anytime about it. Also, how did you find hacking on quadstore?

@gsvarovsky
Copy link
Contributor Author

A lot more fun than doing the market research I've been putting off...

It's a great project, you should be proud!

@gsvarovsky gsvarovsky deleted the key-value-interface branch November 25, 2020 10:32
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.

None yet

2 participants