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.

Sign up for GitHub

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

Jump to bottom

Implement read-snapshots #45

Closed
rvagg opened this issue Oct 28, 2012 · 9 comments
Closed

Implement read-snapshots #45

rvagg opened this issue Oct 28, 2012 · 9 comments
Labels
discussion Discussion enhancement New feature or request

Comments

@rvagg
Copy link
Member

rvagg commented Oct 28, 2012

options.snapshot = NULL; on an iterator will give a consistent snapshot view of the data while reading. db->GetSnapshot(); will create a snapshot and return a version that can be set against options.snapshot. The drawback of the latter is that you have to explicitly db->ReleaseSnapshot() to clean up versions you've created.

At least the NULL case would be handy to start with.
@Raynos
Copy link
Member

Raynos commented Nov 20, 2012

So currently no snapshot is used whilst reading and you get an inconsistent view (i.e. puts after you start reading reflect in your results)

@rvagg
Copy link
Member Author

rvagg commented Nov 20, 2012

actually, sorry, it's NULL by default so any of the ReadStreams will be over a consistent view of the database created by an implicit snapshot.

@dominictarr
Copy link
Contributor

I think this is off by default,
I have a test where a readStream starts in the first tick,
and then later that tick I do some puts into that read's range.

The puts come out in the read stream.
I think when it says "implicit snapshot" it's just referring to an implementation detail.
It's not providing a consistent view.

@rvagg
Copy link
Member Author

rvagg commented Dec 2, 2012

OK, well we need tests for snapshots anyway so a good start would be a test for the current implementation to see what it's actually doing. I'll try and get to that soon.

@rvagg
Copy link
Member Author

rvagg commented Dec 3, 2012

See: Level/levelup@40dbde6#diff-3

You'll need to update your slow-stream in your devDeps to make this one run properly.

Basically the test adds in a bunch of data, then creates a readStream() and then goes ahead and overwrites all the original data with new cruft. The stream is piped through a SlowStream to ensure that it's throttled to be slower than it takes the overwrite to happen.

The test demonstrates that an "implicit snapshot" indeed means that a snapshot is being created to handle the iterator even without being told to do so. You can verify this further with some debug prints on stream 'data' and the return from the second batch() which is done with {sync:true} to be absolutely sure. You can also increase the rate of the SlowStream to something even higher, say 50ms between 'data' events and it should still pass. (And yes, SlowStream does apply back-pressure, I even did some debugging from the C++ layer to ensure that the iterator was being called much slower than it took the batch to happen).

Got any better ideas for a test case on this?

Also on that commit you'll see the basics of some snapshot work on the C++ layer, it should be fairly simple to implement but I need some time to make a nice JS interface and some decent tests for it.

P.S. if you ever get failed tests, then run again and get passed tests, it's probably a database not being deleted properly from the test/ directory. I really need to do a rimraf() on the dir before it's used again just to make sure..

@luk-
Copy link

luk- commented Apr 11, 2013

I'm +1 on snapshots exposed in levelup

@sethyuan
Copy link

Still no support for snapshots?

@rvagg
Copy link
Member Author

rvagg commented May 12, 2014

see #138
marked as "help wanted", it's mostly specced out if someone wants to tackle it but for now I think this should be mostly kept to leveldown rather than being exposed here in levelup

@ralphtheninja ralphtheninja reopened this Dec 18, 2018
@ralphtheninja ralphtheninja transferred this issue from Level/levelup Dec 18, 2018
@vweevers vweevers added discussion Discussion enhancement New feature or request labels Jan 1, 2019
vweevers added a commit to Level/abstract-level that referenced this issue Sep 25, 2022
@vweevers
Add explicit snapshots
49dd8ac 
At the moment, this is a documentation-only PR, acting as an RFC. In
particular I'd like review of my choice to use a token-based
approach (as first suggested by Rod Vagg in
Level/community#45) as opposed to a
dedicated snapshot API surface (as suggested by Julian Gruber in
Level/community#47). Main reasons for not
choosing the latter:

- It would be a third API surface. We have the main database API,
  the sublevel API which is equal to that except for implementation-
  specific additional methods, and would now add another API which
  only has read methods (get, iterator, etc, but not put and batch).
  If the API was reusable, meaning you could pass around a snapshot
  as if it was a regular database (like you can with sublevels) then
  I'd be cool with it. But for that to happen, we'd have to
  implement transactions as well, which I consider to be out of
  scope although transactions are in fact a use case of snapshots.
- It would have a higher complexity once you factor in sublevels.
  I.e. to make `db.sublevel().snapshot().get(key)` read from the
  snapshot but also prefix the given key. By instead doing
  `db.sublevel().get(key, { snapshot })`, the sublevel can just
  forward that snapshot option to its parent database.
- Furthermore, with the token approach, you can pass a snapshot to
  multiple sublevels, which cleanly solves the main use case of
  retrieving data from an index (I included an example in the docs).

I renamed the existing snapshot mechanism to "implicit snapshots"
and attempted to clarify the behavior of those as well.

If accepted, some related issues can be closed, because this PR:

- Includes (a more complete write-up than)
  Level/classic-level#40.
- Mentions Level/leveldown#796 (which
  should be moved rather than closed).
- Answers Level/classic-level#28.
- Solves the main use case as described in
  Level/leveldown#486.
- Effectively completes Level/awesome#19.
This was referenced Sep 25, 2022
Merged
Open
@vweevers
Copy link
Member

Tracked in #118.

@vweevers vweevers mentioned this issue Sep 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion Discussion enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@luk- @dominictarr @ralphtheninja @Raynos @rvagg @vweevers @sethyuan