feat: add context-aware cache key #16
Conversation
aaron-steinfeld
requested review from
avinashkolluru,
surajpuvvada and
tim-mwangi
as code owners
|
whoops - meant to draft! |
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #16 +/- ##
============================================
+ Coverage 67.33% 71.49% +4.15%
- Complexity 64 78 +14
============================================
Files 15 16 +1
Lines 199 228 +29
Branches 9 12 +3
============================================
+ Hits 134 163 +29
+ Misses 57 55 -2
- Partials 8 10 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| public static final Set<String> HEADER_PREFIXES_TO_BE_PROPAGATED = | ||
| Set.of(TENANT_ID_HEADER_KEY, "X-B3-", "grpc-trace-bin", | ||
| "traceparent", "tracestate", AUTHORIZATION_HEADER); | ||
| static final Set<String> CACHE_MEANINGFUL_HEADERS = |
There was a problem hiding this comment.
I know there's some debate whether authorization is meaningful. I'll remove it if needed to unblock this since we can revisit this later in a common way, but I maintain it's better to have the default implementation err on the side of being always correct at the cost of potential duplication.
|
@tim-mwangi @skjindal93 @avinashkolluru - can I please get a review on this? If people would prefer me remove the auth header to defer that debate that's fine, but I'd like to use this elsewhere to prevent making the ~20th implementation of this pattern :) |
| private final T data; | ||
| private final Map<String, String> meaningfulContextHeaders; | ||
|
|
||
| DefaultContextualKey(RequestContext context, T data) { |
There was a problem hiding this comment.
Do we always have to pass in some data to make the request context as a cache key? Can we also have a constructor, which just takes in the RequestContext as an argument?
There was a problem hiding this comment.
I am a bit confused why do we need data (which I believe is an input to the GRPC function calls) as a parameter for the cache key
Basically, why should the contextual key change on a different data?
There was a problem hiding this comment.
Do we always have to pass in some data to make the request context as a cache key? Can we also have a constructor, which just takes in the RequestContext as an argument?
We don't always need to use data, there's a no arg version (this is always built off the context) https://github.com/hypertrace/java-grpc-utils/pull/16/files#diff-ef190554e9c2549e8421789de3b00e6c5fae5294934d883251699dff75676817R87 )
I'll create flavors of the function execution methods that don't provide data, too.
I am a bit confused why do we need data (which I believe is an input to the GRPC function calls) as a parameter for the cache key
Basically, why should the contextual key change on a different data?
So I attempt to convey the distinction by calling a Contextual key rather than a Context key - that is, it's a key that accounts for the context rather than a key for the context. Since the former is a superset of the latter, it felt more flexible to me as it can both handle cases where we solely want a per-context key, but also cases where we, for example, want to cache something like an entity by id.
There was a problem hiding this comment.
Updated the first part.
This comment has been minimized.
This comment has been minimized.
Description
In many places across many repos, we try to solve the problem of caching remote call results in different ways - usually by using the call arguments as a cache key, or sometimes the tenant id and the call arguments. This is meant to provide a reusable way of doing so, that leaves the meaningful parts of the request context as an implementation detail. The idea is that, as more data is added to our request context (for example, user identifiers), that will flow into cache keys without requiring widespread implementation changes.
Testing
Added UTs
Checklist: