kns & app_store: new serialization strategy #620
Conversation
|
Does this fully replace #614 ? |
yes, app_store work pending reviews from that one have been assessed here though |
barraguda
changed the title
| println!( | ||
| "kns_indexer: loaded state: version: {}, last_block: {}, chain_id: {}, kimap_address: {}", | ||
| state.version, | ||
| state.last_block, | ||
| state.get_chain_id(), | ||
| state.get_contract_address() | ||
| ); |
There was a problem hiding this comment.
if this is going to print on every boot it should be aesthetically pleasing
There was a problem hiding this comment.
something like this maybe?
Tue 17:09 kns-indexer:sys:
🐦⬛ KNS Indexer State
▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔
Version 1
Last Block 129424597
Chain ID 10
KIMAP 0xcA92476B2483aBD5D82AEBF0b56701Bb2e9be658
▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁
There was a problem hiding this comment.
line up the number values horizontally but yes :)
| // includes keys and values for: | ||
| // "meta:chain_id", "meta:version", "meta:last_block", "meta:contract_address", | ||
| // "names:{namehash}" -> "{name}", "nodes:{name}" -> "{node_info}" | ||
| kv: Kv<String, Vec<u8>>, // todo: maybe serialize directly into known enum of possible types? |
There was a problem hiding this comment.
can you resolve this TODO before merge? what is the type going in here? it looks like just nodes, and then a handful of static types
There was a problem hiding this comment.
yep, the KV struct currently takes in a type for the value, I'll see if I can relax that requirement in the process_lib with a specific function passing in <K,V> rather than at instantiation.
non-static-types right now: Strings for names and namehashes, net::KnsUpdates for node_infos
There was a problem hiding this comment.
using set_as:: and get_as:: reduced code amount here some, but left the default as Vec instead of having some bigger enum to match and serialize.
barraguda
force-pushed
the
bp/kvstate
branch
from
c95d42b to
e586d5a
Compare
nick1udwig
left a comment
nick1udwig
left a comment
There was a problem hiding this comment.
small question otherwise LGTM
| state | ||
| } | ||
|
|
||
| fn meta_version_key() -> &'static str { |
There was a problem hiding this comment.
Is it best practice to return a static str and then .to_string() it, as we do below?
There was a problem hiding this comment.
probably not, kv helper takes ownership to serialize, so made those helpers just return a String directly instead!
| .send()?; | ||
| } | ||
|
|
||
| IndexerRequest::NodeInfo(NodeInfoRequest { ref name, .. }) => { |
There was a problem hiding this comment.
note: should we add a kimap.get() call here (if we happened to miss an event), or have a separate IndexerRequest for it?
There was a problem hiding this comment.
yes we should. lazy eval/correctness in indexer. also in net runtime module need to do same thing.
Problem
Versions of kns_indexer that keep hashmaps of names in memory (and or serialize that entire state and save it) will at some point run into memory limits and or just performance issues when indexing many events.
Same goes for app_store.
Solution
This PR uses key value as a hashmap on disk for KNS, doing gets and sets directly to disk.
For app_store, it uses the sqlite runtime module for better querying capabilities in the future too,
Notes
Some things that will still be added: