android: add support for registering a platform KV store by goaway · Pull Request #2134 · envoyproxy/envoy-mobile

goaway · 2022-03-29T20:02:01Z

Description: This PR allows platform implementations of a generic (and potentially persistent) key-value store to be registered with the Engine. Internal components can look up these implementations by name. The internal implementation and wiring in this PR is platform agnostic, but this PR adds a public API for Java/Kotlin only, initially. iOS and other public APIs to follow in upcoming PRs.

Part of #2077.

Risk Level: Low
Testing: New Unit + Integration Coverage

Signed-off-by: Mike Schore mike.schore@gmail.com

Signed-off-by: Mike Schore <mike.schore@gmail.com>

RyanTheOptimist

Nice! A few small comments. I think it would also be nice to flesh out the PR description to explain a bit more the details of what's going on.

RyanTheOptimist · 2022-05-12T14:01:21Z

library/common/extensions/filters/http/test_kv_store/config.cc

+namespace Envoy {
+namespace Extensions {
+namespace HttpFilters {
+namespace TestKeyValueStore {


I would have expected the name of this to be something like PlatformKeyValueStore. Is this filter intended to be used in production? Maybe it's just test-only?

Your presumption is correct - this is a test-only filter that exist alongside some other test-only filters in an entirely unintuitive location. Ideally, these should be moved elsewhere. I can certainly add a clarifying comment.

RyanTheOptimist · 2022-05-12T14:10:42Z

library/java/io/envoyproxy/envoymobile/engine/types/EnvoyKeyValueStore.java

+   * Remove a value from the key value store implementation.
+   *
+   * @param key,     key identifying the value to be removed.
+   * @return String, value mapped to the key, or null if not present.


nit: Looks like this returns void?

RyanTheOptimist · 2022-05-12T14:10:47Z

library/java/io/envoyproxy/envoymobile/engine/types/EnvoyKeyValueStore.java

+   * Save a value to the key value store implementation.
+   *
+   * @param key,     key identifying the value to be saved.
+   * @param value,   the value to be saved.


nit: Looks like this returns void?

I think this it still outstanding.

Sorry I may be being dense - what's the fix here? I fixed the @return for remove, but this method doesn't have one.

Oh, shoot. You're right. I somehow totally misread this. :(

All good - thanks for all the comments. :)

RyanTheOptimist · 2022-05-12T14:26:28Z

library/common/extensions/filters/http/test_kv_store/filter.h

+
+using TestKeyValueStoreFilterConfigSharedPtr = std::shared_ptr<TestKeyValueStoreFilterConfig>;
+
+class TestKeyValueStoreFilter final : public ::Envoy::Http::PassThroughFilter {


nit: Please add a comment which describes what this filter does.

RyanTheOptimist · 2022-05-12T14:30:26Z

library/common/extensions/key_value/platform/config.cc

+    envoy_data bridged_value = Data::Utility::copyToBridgeData(contents);
+    bridged_store_.save(bridged_key, bridged_value, bridged_store_.context);
+  }
+


I would have expected a remove() method. Is that not required?

@alyssawilk shared that you all didn't have a use case for it right now (which I sort of found surprising, but then again, if you supply the KV store implementation, you can always remove stuff from it elsewhere). I went ahead and wired remove at the other layers anyways, simply because I do expect we will have a use for it.

RyanTheOptimist · 2022-05-12T14:31:42Z

library/common/jni/jni_interface.cc

+  jobject j_context = static_cast<jobject>(const_cast<void*>(context));
+
+  jclass jcls_JvmKeyValueStoreContext = env->GetObjectClass(j_context);
+  jmethodID jmid_read = env->GetMethodID(jcls_JvmKeyValueStoreContext, "read", "([B)[B");


It's too bad there is no magic for auto-generating these method signature strings, but c'est la vie.

In fact, there is! ...but I never figured out how it might work with Bazel. But javac itself has support for generating jni headers based on native method signatures in class definitions in java.

Yeah, this is bound to bite us at some point. cxx.rs automatically generates these which helps avoid typos. If we start introducing Rust at this layer it would be worth considering using it for JNI codegen.

goaway · 2022-05-16T06:54:50Z

Nice! A few small comments. I think it would also be nice to flesh out the PR description to explain a bit more the details of what's going on.

👍

Signed-off-by: Mike Schore <mike.schore@gmail.com>

RyanTheOptimist

LGTM, modulo one nit.

RyanTheOptimist · 2022-05-16T16:16:53Z

library/java/io/envoyproxy/envoymobile/engine/types/EnvoyKeyValueStore.java

+   * Save a value to the key value store implementation.
+   *
+   * @param key,     key identifying the value to be saved.
+   * @param value,   the value to be saved.


I think this it still outstanding.

library/common/extensions/filters/http/test_kv_store/BUILD

jpsim · 2022-05-16T20:32:12Z

library/common/extensions/key_value/platform/c_types.h

+/**
+ * Function signature for reading value from implementation.
+ */
+typedef envoy_data (*envoy_kv_store_read_f)(envoy_data key, const void* context);


Is there a reason you went with a synchronous approach here? Is the idea that consumers would take care of moving this to a separate thread if they don't want to block on reading/writing?

Well, for read asynchronous would introduce a whole new calling paradigm ("onRead"?). Honestly, I'd think an implementation should probably do caching/pre-caching/memoization if read performance is likely to be a bottleneck somewhere. For write/remove , yes, I think that's also better off as a feature of the actual provided platform implementation.

ok. Another thing to consider is whether or not errors should be propagated between the platform and native layers.

jpsim · 2022-05-16T20:33:07Z

library/common/extensions/key_value/platform/c_types.h

+/**
+ * Function signature for saving value to implementation.
+ */
+typedef void (*envoy_kv_store_save_f)(envoy_data key, envoy_data value, const void* context);


Can this document what's expected as the context pointer?

I added a more general description to the root c_types.h, since we use these everywhere.

jpsim · 2022-05-16T20:35:49Z

library/common/jni/jni_interface.cc

+}
+
+static void jvm_kv_store_remove(envoy_data key, const void* context) {
+  jni_log("[Envoy]", "jvm_store_remove");


Nit:

Suggested change

jni_log("[Envoy]", "jvm_store_remove");

jni_log("[Envoy]", "jvm_kv_store_remove");

Ah, good catch.

jpsim · 2022-05-16T20:44:08Z

library/proguard.txt

   <methods>;
 }

+-keep, includedescriptorclasses class io.envoyproxy.envoymobile.engine.JvmKeyValueStoreContext {


How is this file updated? Asking so I know when/how to do this next time I touch the Java implementation.

Manually, when a new JNI-facing class is added.

Signed-off-by: Mike Schore <mike.schore@gmail.com>

jpsim · 2022-05-16T20:50:23Z

Can you please add a changelog entry for the user-facing change?

Signed-off-by: Mike Schore <mike.schore@gmail.com>

jpsim · 2022-05-16T20:54:06Z

library/common/types/c_types.h

 // NOLINT(namespace-envoy)

+/**
+ * Throughout this file one may note that most callbacks take a void* context parameter, and most


👌 excellent comment

jpsim

Nice work. Looking forward to seeing the default persistent kv store too.

* origin/main: api: disallow setting 'host' header directly (#2275) android: add support for registering a platform KV store (#2134) Bump Lyft Support Rotation (#2278) Signed-off-by: JP Simard <jp@jpsim.com>

…atcher-again * main: envoy: update to d88f31b (#2279) api: disallow setting 'host' header directly (#2275) android: add support for registering a platform KV store (#2134) Bump Lyft Support Rotation (#2278) tools: Enable the VSCode completion db to use bazelisk if available (#2277) Signed-off-by: JP Simard <jp@jpsim.com>

* origin/main: (97 commits) docs: update python packages to work with Python 3.10 (#2286) test: adding a cancel test, cleaning up copy-paste code (#2283) envoy: update to d88f31b (#2279) api: disallow setting 'host' header directly (#2275) android: add support for registering a platform KV store (#2134) Bump Lyft Support Rotation (#2278) tools: Enable the VSCode completion db to use bazelisk if available (#2277) Release v0.4.6.20220513-4 Fix android_release_deploy Release v0.4.6.20220513-3 Release v0.4.6.20220513-2 net: enable happy eyeballs by default (#2272) git: avoid merge conflicts when adding changelog entries (#2273) docs: fix sphinx reference mismatch warning (#2274) tests: add -Xcheck:jni to kotlin integration tests by default (#2269) configuration: enable h2 ping by default (#2270) Add version history entries for user-facing changes (#2271) configuration: filter unroutable addresses on Android by default (#2267) Integrate rules_xcodeproj (#2263) Add assert when failing to get_env (#2253) ... Signed-off-by: JP Simard <jp@jpsim.com>

goaway added 2 commits March 31, 2022 02:30

android: add support for registering a platform KV store

9b328f9

Signed-off-by: Mike Schore <mike.schore@gmail.com>

wiring

a5c1612

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway force-pushed the ms/java-kv-store branch from bd5669e to a5c1612 Compare March 30, 2022 18:30

goaway added 9 commits April 4, 2022 20:17

test fixes

3c49ae3

Signed-off-by: Mike Schore <mike.schore@gmail.com>

format

f1fdadf

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add c types

0781580

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add bridged impl

6c4c983

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix bridge wiring and jni wip

051da27

Signed-off-by: Mike Schore <mike.schore@gmail.com>

jni wip

0630c43

Signed-off-by: Mike Schore <mike.schore@gmail.com>

jni wiring

972c5d8

Signed-off-by: Mike Schore <mike.schore@gmail.com>

format

a491b1a

Signed-off-by: Mike Schore <mike.schore@gmail.com>

spelling fix

e57b637

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway marked this pull request as ready for review April 13, 2022 10:40

goaway changed the title ~~[wip] android: add support for registering a platform KV store~~ android: add support for registering a platform KV store Apr 13, 2022

goaway added 4 commits April 21, 2022 17:55

Merge branch 'main' into ms/java-kv-store

66c4335

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Merge branch 'main' into ms/java-kv-store

e970387

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Merge branch 'main' into ms/java-kv-store

3726e3e

Signed-off-by: Mike Schore <mike.schore@gmail.com>

test fix

2f6c5bf

Signed-off-by: Mike Schore <mike.schore@gmail.com>

jpsim self-assigned this Apr 29, 2022

goaway added 7 commits May 6, 2022 01:41

Merge branch 'main' into ms/java-kv-store

4f4fa7f

Signed-off-by: Mike Schore <mike.schore@gmail.com>

test wip

0268652

Signed-off-by: Mike Schore <mike.schore@gmail.com>

Merge branch 'main' into ms/java-kv-store

784415a

Signed-off-by: Mike Schore <mike.schore@gmail.com>

passing integration test

287dd84

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix merge conflict

0d6ee96

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix ReceiveDataTest

8a875ae

Signed-off-by: Mike Schore <mike.schore@gmail.com>

remove logging cruft

d0fe4f4

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway requested review from Augustyniak, RyanTheOptimist and alyssawilk May 12, 2022 10:16

format

7bb85fe

Signed-off-by: Mike Schore <mike.schore@gmail.com>

RyanTheOptimist reviewed May 12, 2022

View reviewed changes

RyanTheOptimist self-assigned this May 13, 2022

goaway added 2 commits May 16, 2022 15:02

add description to TestKeyValueStore filter

887da7c

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix docstring

208e3ff

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway requested a review from RyanTheOptimist May 16, 2022 07:06

Merge branch 'main' into ms/java-kv-store

e9a1626

Signed-off-by: Mike Schore <mike.schore@gmail.com>

RyanTheOptimist approved these changes May 16, 2022

View reviewed changes

jpsim reviewed May 16, 2022

View reviewed changes

library/common/extensions/filters/http/test_kv_store/BUILD Show resolved Hide resolved

jpsim reviewed May 16, 2022

View reviewed changes

RyanTheOptimist previously approved these changes May 16, 2022

View reviewed changes

jpsim reviewed May 16, 2022

View reviewed changes

add description of context fields/params to c_types.h

d64efab

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway dismissed RyanTheOptimist’s stale review via d64efab May 16, 2022 20:49

goaway added 2 commits May 17, 2022 04:50

fix typo in log

e6f609d

Signed-off-by: Mike Schore <mike.schore@gmail.com>

update version history

de0a34e

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway requested a review from jpsim May 16, 2022 20:53

jpsim reviewed May 16, 2022

View reviewed changes

jpsim approved these changes May 16, 2022

View reviewed changes

goaway merged commit b309613 into main May 17, 2022

goaway deleted the ms/java-kv-store branch May 17, 2022 00:59


		using TestKeyValueStoreFilterConfigSharedPtr = std::shared_ptr<TestKeyValueStoreFilterConfig>;

		class TestKeyValueStoreFilter final : public ::Envoy::Http::PassThroughFilter {

	jni_log("[Envoy]", "jvm_store_remove");
	jni_log("[Envoy]", "jvm_kv_store_remove");

Conversation

goaway commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goaway commented May 16, 2022

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpsim May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpsim commented May 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpsim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

goaway commented Mar 29, 2022 •

edited

Loading

jpsim May 16, 2022 •

edited

Loading