-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add in-memory cache with ttl to reduce hash time computation #4693
base: master
Are you sure you want to change the base?
Conversation
a1b44bf
to
364e11e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
were you able to run a performance test scenario to check the results ?
364e11e
to
8de0470
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
a99441d
to
f7b8d41
Compare
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