[leveldb_proto] Consider adding a way to load both keys with entries |
||||
Issue descriptionChrome Version: M71 (HEAD) OS: All What steps will reproduce the problem? (1) Have a set of data where the key is a significant piece and that is often used other places as well. (2) Try to load the data, when you need both keys and proto. What is the expected result? I get both keys and values from LoadEntries*() or some methods of the API. What happens instead? LoadEntries* only returns entries, not keys. You can know the key for each entry by loading one at a time, but that's not efficient. Other notes? We should investigate whether it makes sense to have methods along the lines of LoadKeysAndEntries*(), returning a KeyEntryVector instead of just std::vector<T>. Basically, we have LoadKeys() and LoadEntries(), and the latter has support for filters. But there's no way of knowing the key for each entry unless you load one at a time. We should also consider whether the overhead of forcing all usages of the API to also return keys is reasonable enough to continue to only support a single API for loading entries, or whether we should also keep the current API.
,
Sep 12
This forced us to store the key in the proto, so having this addition to the API would be awesome!
,
Sep 12
To be honest, I believe there are many places where we unnecessarily store the key with the value. I would kind of want us to remove the old API though, even though it temporarily might cause a small amount of memory increase during load.
,
Sep 17
I'll take this since I'm working with this code extensively these days.
,
Sep 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d3c43f67173dbb48ef680a1a51553f9519c52988 commit d3c43f67173dbb48ef680a1a51553f9519c52988 Author: Troy Hildebrandt <thildebr@chromium.org> Date: Fri Sep 28 19:20:12 2018 Add ability to load keys/entries at same time in LevelDB/ProtoDB. Keys and entries can currently be loaded separately, but never at the same time, meaning it requires separate calls to LoadKeys/LoadEntries to get keys and entries. Some users of ProtoDatabase have resorted to storing the keys in the entries to get around this. This CL adds LoadKeysAndEntries(WithFilter) that returns both at the same time. ProtoDatabase's new LoadKeysAndEntriesCallback gives back a vector of string/proto pairs. Bug: 883409 Change-Id: Id624bbdeb356395c6ce679528d85c1f934a9772e Reviewed-on: https://chromium-review.googlesource.com/1228305 Commit-Queue: Troy Hildebrandt <thildebr@chromium.org> Reviewed-by: Tommy Nyquist <nyquist@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#595175} [modify] https://crrev.com/d3c43f67173dbb48ef680a1a51553f9519c52988/components/leveldb_proto/leveldb_database.cc [modify] https://crrev.com/d3c43f67173dbb48ef680a1a51553f9519c52988/components/leveldb_proto/leveldb_database.h [modify] https://crrev.com/d3c43f67173dbb48ef680a1a51553f9519c52988/components/leveldb_proto/proto_database.h [modify] https://crrev.com/d3c43f67173dbb48ef680a1a51553f9519c52988/components/leveldb_proto/proto_database_impl.h [modify] https://crrev.com/d3c43f67173dbb48ef680a1a51553f9519c52988/components/leveldb_proto/proto_database_impl_unittest.cc [modify] https://crrev.com/d3c43f67173dbb48ef680a1a51553f9519c52988/components/leveldb_proto/testing/fake_db.h
,
Oct 9
|
||||
►
Sign in to add a comment |
||||
Comment 1 by nyquist@chromium.org
, Sep 12