fix: add in-memory cache with ttl to reduce hash time computation #4693
Conversation
guillaumelamirand
force-pushed
the
optimize-client-identifier-computation
branch
from
a1b44bf
to
364e11e
Compare
guillaumelamirand
force-pushed
the
optimize-client-identifier-computation
branch
from
364e11e
to
8de0470
Compare
guillaumelamirand
force-pushed
the
optimize-client-identifier-computation
branch
4 times, most recently
from
d6110df
to
a99441d
Compare
.../io/gravitee/gateway/reactive/handlers/api/processor/subscription/SubscriptionProcessor.java
Show resolved
Hide resolved
| return fallback; | ||
| } | ||
| } | ||
|
|
||
| private Cache<String, String> getOrCreateCache() { | ||
| return cacheManager.getOrCreateCache( | ||
| REMOTE_ADDRESS_HASHES_CACHE, |
There was a problem hiding this comment.
maybe use the classname so it is unique for sure.
| private Cache<String, String> getOrCreateCache() { | ||
| return cacheManager.getOrCreateCache( | ||
| REMOTE_ADDRESS_HASHES_CACHE, | ||
| CacheConfiguration.builder().timeToLiveInMs(60_000).timeToIdleInMs(60_000).maxSize(1000).distributed(false).build() |
There was a problem hiding this comment.
1 min is quite short. I understand there might be a lot of request. How did you arrive to the conclusion that 1 minute is the best value. I'm not asking a new parameter.
There was a problem hiding this comment.
The idea is to only limit the computation during burst requests
.../io/gravitee/gateway/reactive/handlers/api/processor/subscription/SubscriptionProcessor.java
Outdated
Show resolved
Hide resolved
| @@ -289,6 +307,7 @@ void should_suffix_client_identifier_header_with_hash_when_subscription_equals_r | |||
| verify(mockRequest).clientIdentifier(AdditionalMatchers.not(eq(TRANSACTION_ID))); | |||
| verify(mockRequest).clientIdentifier(AdditionalMatchers.not(eq(REMOTE_ADDRESS))); | |||
| assertThat(spyCtx.metrics().getClientIdentifier()).isEqualTo(clientIdentifier); | |||
| assertThat(cacheManager.getOrCreateCache(REMOTE_ADDRESS_HASHES_CACHE).get(REMOTE_ADDRESS)).isNotNull(); | |||
There was a problem hiding this comment.
You don't test TTL, I'm saying test the cache, I'm saying test that TLL works (maybe you need to tweak you class so you can change TTL values)
There was a problem hiding this comment.
I suppose you missed a word. Anyway ttl of the cache is tested on the cache module itself. Do you want to wait 1minute to make sure the value is evicted ?
guillaumelamirand
force-pushed
the
optimize-client-identifier-computation
branch
from
a99441d
to
f7b8d41
Compare
guillaumelamirand
force-pushed
the
optimize-client-identifier-computation
branch
from
f7b8d41
to
c80f87c
Compare
|
On hold waiting for 4.2 release. |
|
@guillaumelamirand 4.2 (and 4.3) has been released. Should we consider updating this PR? Or can we close it? |
Yes, the idea was to wait the 4.2 to have the time to properly test the fixe. |
Description
When using a keyless plan, the remote address is used as a client identifier. To avoid divulging sensitive data, we hash it before. However, that is CPU consuming and so the idea here is to add a in-memory cache with short ttl to deal with a burst of requests.
Additional context
馃摎聽聽View the storybook of this branch here