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

Update Datastore Interface #36

Merged
merged 3 commits into from Dec 3, 2019
Merged

Update Datastore Interface #36

merged 3 commits into from Dec 3, 2019

Conversation

aschmahmann
Copy link
Contributor

Per ipfs/go-datastore#140 the Datastore interface is being updated to support asynchronous writes.

This PR should be approved before the above PR is merged. After ipfs/go-datastore#140 is merged then this PR should have its go.mod updated and merged.

A couple quirks about adding the Sync(prefix) function to this LevelDB wrapper:

  1. It looks like our Puts and Deletes have always been asynchronous and we just didn't realize or do anything about it
    • We could use a global flag to decide whether to pass WriteOptions.Sync which would restore the behavior we thought we had
  2. We need to figure out how to actually efficiently call journal sync on subkeys if we want to conform to the interface (despite us not currently doing that)

@Stebalien
Copy link
Member

It looks like our Puts and Deletes have always been asynchronous and we just didn't realize or do anything about it

Hm. Yeah, we should definitely make it sync by default. Thanks for catching this!

We need to figure out how to actually efficiently call journal sync on subkeys if we want to conform to the interface (despite us not currently doing that)

The guarantee of the interface is that all keys under the prefix will be synced, not that other keys won't also be synced. I'd expect most datastores to just sync everything.

@Stebalien
Copy link
Member

We need to figure out how to actually efficiently call journal sync on subkeys if we want to conform to the interface (despite us not currently doing that)

Ah. I see. There's no way to actually call sync other than to write a new value.

@Stebalien
Copy link
Member

syndtr/goleveldb#310

@aschmahmann
Copy link
Contributor Author

Ah. I see. There's no way to actually call sync other than to write a new value.

That's what it looks like, but some of the internals are a little confusing. It seems like if the thing that writes the journal implements Flush() that it's called even if the sync option is not passed into the Write

https://github.com/syndtr/goleveldb/blob/758128399b1df3a87e92df6c26c1d2063da8fabe/leveldb/db_write.go#L26

However, I don't think there are any provided journal writers that implement Flush.

A few options:

  1. Do some more investigating into if/when things are actually flushed/synced
  2. Just make this a Sync database, have Sync() return nil, file an issue and move on
  3. Have the Async version of the database do a sync write to a preset special key (e.g. "syncFlagDoNotUse") when we want to sync

Thoughts?

@Stebalien
Copy link
Member

It seems like if the thing that writes the journal implements Flush() that it's called even if the sync option is not passed into the Write

Flush and Sync aren't quite the same thing. Flush means "flush the buffers" to the OS. Sync means "wait for the OS to write this to disk".

Just make this a Sync database, have Sync() return nil, file an issue and move on

For now, let's do this. If syndtr/goleveldb#310 gets merged, we can reconsider.

@@ -224,6 +231,6 @@ func (d *Datastore) NewTransaction(readOnly bool) (ds.Txn, error) {
if err != nil {
return nil, err
}
accessor := &accessor{tx}
accessor := &accessor{ldb: tx, syncWrites: false}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

false was chosen here because it's the default behavior and since the internal transaction totally ignores WriteOptions anyway.

@aschmahmann aschmahmann marked this pull request as ready for review December 3, 2019 16:01
@aschmahmann aschmahmann merged commit f885e89 into master Dec 3, 2019
@aschmahmann aschmahmann deleted the feat/ds-update branch December 3, 2019 21:37
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