-
Notifications
You must be signed in to change notification settings - Fork 31
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
onlyIfChanged: adds option to put only if value passed is different from value at key #47
Conversation
test/all.js
Outdated
tape('put onlyIfChanged inserts only if existing value !== value', async t => { | ||
const Hypercore = require('hypercore') | ||
const feed = new Hypercore(require('random-access-memory')) | ||
const db = new Hyperbee(feed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the create helper above
test/all.js
Outdated
tape('batch put onlyIfChanged inserts only if existing value !== value', async t => { | ||
const Hypercore = require('hypercore') | ||
const feed = new Hypercore(require('random-access-memory')) | ||
const db = new Hyperbee(feed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
test/all.js
Outdated
const Hypercore = require('hypercore') | ||
const feed = new Hypercore(require('random-access-memory')) | ||
const db = new Hyperbee(feed) | ||
await db.ready() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
test/all.js
Outdated
const Hypercore = require('hypercore') | ||
const feed = new Hypercore(require('random-access-memory')) | ||
const db = new Hyperbee(feed) | ||
await db.ready() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed
@@ -622,6 +634,15 @@ class Batch { | |||
node = await node.getChildNode(i) | |||
} | |||
|
|||
if (onlyIfChanged) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure this would never fire. the only time an existing key is found is above in c === 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually meant to add a question about this. In my testing I never triggered the loop, is there a branching factor in the bee tree that delays splits until a certain number of keys have been written? In my testing this was the branch that was exclusively hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the split is only relevant for maintaining the b-tree index (see tree splitting with b-trees on wikipedia for example), so so a "put(key) when key exists" should ALWAYS hit the while above, ie it has to find the existing node first.
if (onlyIfChanged) { | ||
const block = await this.getBlock(node.keys[mid].seq) | ||
const same = (Buffer.compare(block.value, value) === 0) | ||
if (same) try { return block.final() } finally { this._unlockMaybe() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (same) return this._unlockMaybe()
is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will violate the expected behavior once #46 is merged, returning block.final()
satisfies the "new" expectation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's fine, we'll tweak after or fix in the other PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
test/all.js
Outdated
const value = Buffer.from('value') | ||
await db.put(key, value) | ||
const fst = await db.get(key) | ||
const fstlen = db._feed.length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db.feed should be fine instead of _feed (everywhere here)
Not needed now because of #50 |
This PR adds an
{ onlyIfChanged: Boolean }
option toHyperbee.put()
andHyperbee.batch().put()
, constraining puts at a key to only succeed if the existing value at the key is not the same (i.e. if the encoded binary values are not the same).