Support Asynchronous Writing Datastores #140
Conversation
|
After this PR is merged can we have a version bump according to semver practices since this is a breaking change to the |
We will bump the minor version because the package version is sub-zero. |
Won't break |
|
|
||
| if mountPts[i].Equal(prefix) || suffix.String() != "/" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Hm. This looks like a bug in lookupAll. lookupAll should only return mounts that can contain descendents of key.
There was a problem hiding this comment.
We can merge this as-is and fix it later or move this logic into lookupAll now.
There was a problem hiding this comment.
Could you clarify what you think the bug is here? It looks like the documented behavior for lookupAll.
There was a problem hiding this comment.
lookupAll is supposed to return all datastores that might contain keys that are descendants of the given key. That means we should be calling Sync on all datastores returned by lookupAll datastores and we shouldn't have this extra check.
However, if we have ["/foo", "/foo/bar"], "/foo" should "cover" "/foo/bar" given that lookup will always return "/foo". However, when given the prefix "/foo", lookupAll will return both.
Note: this is only technically a bug. Nobody would ever mount "/foo" over "/foo/bar".
There was a problem hiding this comment.
Nobody would ever mount "/foo" over "/foo/bar"
I thought it was expected behavior that we might mount "/" and also "/foo", or is the root a special case?
There was a problem hiding this comment.
My first example was bad: order matters. If we mount "/" over "/foo", "/" will win (I think? check me here). In go-ipfs, we mount "/blocks" over "/".
There was a problem hiding this comment.
I say "nobody would ever mount "/foo" over "/foo/bar" because "/foo" is a prefix of "/foo/bar" (and we check mounts in-order).
There was a problem hiding this comment.
I was wrong. We reverse-sort datastores so "/foo/bar" always comes before "/foo".
However, the bug was valid. Fix in #141.
aschmahmann
force-pushed
the
feat/async-ds
branch
from
d7a00b4
to
b6f1f99
Compare
aschmahmann
force-pushed
the
feat/async-ds
branch
from
b6f1f99
to
8ddf6ad
Compare
|
LGTM but let's merge this tomorrow. Hitting merge then going to bed is a bad idea (I mean, it's one of my favorite bad ideas but I think it's time to call it a day). |
Per #137 we would like to support Datastores that internally have asynchronous writes. We are doing this by adding a
Sync(prefix ds.Key)function to the Datastore interface.The plan is:
Sync()command on their datastores