key_value: structure for prefs based key value store#2120
key_value: structure for prefs based key value store#2120alyssawilk merged 6 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Thanks for creating this @alyssawilk. Instead of callling it "PrefsBased", I'd suggest the name "PlatformKeyValueStore" - this is what we've named other components that have a platform-level implementation that they delegate to. This will need a configurable string identifier for the platform API - this is how we discover platform APIs at runtime. |
|
renamed. can you tell me a bit more about the string identifier, or point to examples? |
|
ping! |
|
PlatformBridgeFilters are maybe the best (if also somewhat complicated) example of this. See: An C API that represents the implementation is defined and and the actual delegating calls are registered (as part of engine initialization) in a global map called the Api::Registry. Lookup of the implementing calls occurs using a unique name for the filter implementation (there are obviously some runtime shortcomings here, but this can all be improved). Filters are complicated, because a platform-level object actually gets instantiated each time the Envoy filter chain does. For a durable store, this is probably unnecessary and the same implementation can be re-used for all calls. |
|
At the end of the day, the important thing is a token that can be specified in config, which can then be used both to register the API during runtime initialization, and to look up the API in compiled extension code. Having thought about it more, I suppose we could use a single static token for this, but the parallel that struck me with filters is that you might actually have or want multiple implementations of this (e.g. preferences vs. secure storage). |
|
IMO it'd be really helpful to have a single static token for now, since the factory that creates the k-v store doesn't have access to the hooks for up calls. or we could add that string to the prefs proto, if folks want to customize. I think that'd be part of your part of the implementation though? |
|
@alyssawilk All it would need though is a single string field in its config proto though, right? |
| class PlatformInterface { | ||
| public: | ||
| virtual ~PlatformInterface() {} | ||
| virtual void flush(const std::string& key, const std::string& contents) PURE; |
There was a problem hiding this comment.
nit: I would suggest one of store, save, write or put as opposed to flush probably.
| envoymobile::extensions::key_value::platform::PlatformKeyValueStoreConfig>( | ||
| typed_config.config().typed_config(), validation_visitor); | ||
| auto milliseconds = | ||
| std::chrono::milliseconds(DurationUtil::durationToMilliseconds(file_config.flush_interval())); |
There was a problem hiding this comment.
While I understand the value in supporting periodic flushes, would it make sense for the base case to be synchronous/immediate?
There was a problem hiding this comment.
I don't think we want to for any of the current use cases - flushing on every dns resolution or every response headers seems expensive. @RyanTheOptimist for thoughts
There was a problem hiding this comment.
Yeah for a per-request or per-DNS resolution store, immediate sync would be too expensive. Chrome's analogous system uses periodic writes, fwiw. I could imagine that we might have other use cases for the key-value store for which immediate write might be desirable so maybe there is value in having that as an option. But for the uses we currently have, periodic flushes make sense to me.
There was a problem hiding this comment.
Okay that's fine - you all are the primary users of this right now anyways. I just thought it might make for easier testing and/or open up other simple use cases that aren't as perf sensitive.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
* main: (59 commits) Bump Lyft Support Rotation (#2156) add specifying more maven deps (#2151) update envoy@e4eaf1b97 (#2146) bazel: create symbol mapping file (#2126) Bump Lyft Support Rotation (#2148) bazel: remove sandbox disable build: export flatbuffer jvm dep (#2147) Bump Lyft Support Rotation (#2143) bazel: Add flatbuffers Swift hack key_value: structure for prefs based key value store (#2120) build: add flatbuffers (#2133) Bump Lyft Support Rotation (#2131) envoy: bump upstream Envoy to 419e237 (#2132) stats: enable more metrics (#2130) Use the right type (envoy_network_t) (#2125) Bump Lyft Support Rotation (#2118) Update CONTRIBUTING.md to include updating subrepos (#2023) ci: create baseline and experimental test app pipelines (#2075) config: temporarily hardcode h2 max concurrent streams to 100 (#2124) ... Signed-off-by: JP Simard <jp@jpsim.com>
3 participants
Part of #2077