-
Notifications
You must be signed in to change notification settings - Fork 65
feat: iterate entries to sync from LMDB store (FOR-139) #460
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
Conversation
|
84faa1f
to
df1611f
Compare
src/hb_store_fs.erl
Outdated
list(Opts, Path) -> | ||
case file:list_dir(add_prefix(Opts, Path)) of | ||
{ok, Files} -> {ok, lists:map(fun hb_util:bin/1, Files)}; | ||
{ok, Files} -> |
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 think the need for this arrises more from a default config issue than the code? hb_opts
is currently defining cache-mainnet/lmdb
as the default, then a fallback FS store at cache-mainnet
IIRC. Better to change that config (FS store => cache-mainnet/fs
?) than to add code here.
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.
done
src/hb_store_lmdb.erl
Outdated
-spec list(map(), binary()) -> {ok, [binary()]} | {error, term()}. | ||
list(Opts, <<"/">>) -> | ||
#{ <<"db">> := DBInstance } = find_env(Opts), | ||
case elmdb:match_prefix(DBInstance, <<"group:">>) of |
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.
Any reason to change the LMDB Rust side from :list
to match_prefix
? When I first read this I assumed we meant matching the value with a prefix, which could be much more expensive?
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.
hey @samcamwilliams , the reason for the match_prefix
and the new KV with group:
prefix on the key is because we need a way to index the groups to prevent iterating the whole database to get the groups.
It follows this approach in order to satisfy the requirement to have a generic sync based on hb_store
behaviour.
If we could have more specialized implementations we could adopt another strategy like iterating all the keys from source store and copying them to the destination store.
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.
removed match_prefix in favor of the iterator/fold
src/hb_store_lmdb.erl
Outdated
make_group(Opts, GroupName) when is_map(Opts), is_binary(GroupName) -> | ||
write(Opts, GroupName, <<"group">>); | ||
write(Opts, GroupName, <<"group">>), | ||
write(Opts, <<"group:", GroupName/binary>>, <<"">>); |
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.
Why make another new key? Is there a way around this? The line 442 is already a bit of a nuisance (the existence of ID/key
s in the DB implies that ID
is a group), but is passable. Adding 2 extra keys to the DB per group will add complexity long-term, I think.
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 have left it to allow a single call for hb_store:type
on groups. But as we've just discussed, we intend to apply a global iterator approach. cc @twilson63
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.
removed once we can get the next process with the iterator
Left some comments! |
b1aed7e
to
9f12523
Compare
9ef8ab2
to
5683f90
Compare
dbc143f
to
9898629
Compare
09a7c20
to
9ef8ab2
Compare
9898629
to
51c711b
Compare
This is important as the database is opened with NO_SYNC
748b30d
to
6013e0c
Compare
With it, iterate_start and iterate_cont are no more exported
6013e0c
to
da05265
Compare
@samcamwilliams This gonna be helpful to merge hydration results from different push machines and to cleanup/reset certain processes. I have addressed all the comments and merged to the base branch |
What/Why
This PR is a complement for #440.
It enables the sync from a LMDB store to any other by allowing to iterate through out all LMDB key values (visiting all the branches and leaves for all hashpath trees).
Additional notes
It depends on twilson63/elmdb-rs#12