Conversation
| @Test | ||
| @DisplayName("Expect 200 status code when endpoint registered.") | ||
| void callUndefinedRoute() { | ||
| given() |
There was a problem hiding this comment.
this test is obsolete, the same action is invoked by the Docker
| this.payloadKey = getPayloadKey(); | ||
|
|
||
| JsonObject cacheConfig = config.getJsonObject("cache"); | ||
| ttl = cacheConfig != null |
| } | ||
|
|
||
| public static String serializeObject(Object object) { | ||
| if (object instanceof JsonObject) { |
There was a problem hiding this comment.
you can use .toString 💡
since toString implementations of JsonArray and JsonObject both invoke their own .encode methods
| @@ -0,0 +1,55 @@ | |||
| [![][license img]][license] | |||
There was a problem hiding this comment.
you can remove those two lines.
| New value cached under key: the-book for 20 seconds | ||
| ``` | ||
|
|
||
| [license]:https://github.com/Cognifide/knotx/blob/master/LICENSE |
There was a problem hiding this comment.
You can remove lines below.
|
|
||
| @Test | ||
| @DisplayName("Expect 200 status code when endpoint registered.") | ||
| void callUndefinedRoute() { |
There was a problem hiding this comment.
Please fix the method name if this test is still valid.
|
|
||
| <logger name="io.knotx" level="DEBUG"/> | ||
| <logger name="io.vertx" level="DEBUG"/> | ||
| <logger name="com.codahale.metrics" level="TRACE"/> |
There was a problem hiding this comment.
Not sure why we copy those logger. Please remove it.
| config { | ||
| redis.host = redis #it's available under this host in docker compose/swarm | ||
| cache.ttl = 20 | ||
| cacheKey = "the-book" |
There was a problem hiding this comment.
| cacheKey = "the-book" | |
| cacheKey = the-book |
| } | ||
|
|
||
| private RedisOptions getRedisOptions(JsonObject config) { | ||
| return Optional.ofNullable(config.getJsonObject("redis")) |
There was a problem hiding this comment.
Can we load Redis options directly from JSON? Currently we are not able to set many options such as encoding, authentication etc.
I would see in configuration
redisOptions {
host = "some value"
...
}
And then we would use the RedisOptions(JsonObject json) constructor to fill all fields. It comes with one additional benefit. The new versions of Vert.x Redis library can introduce new options. We would not have to modify this code when we would use JSON.
| private final Action doAction; | ||
|
|
||
| RedisCacheAction(Vertx vertx, JsonObject config, Action doAction) { | ||
| this.config = config; |
There was a problem hiding this comment.
Can we introduce RedisCacheActionOptions ? Then all defaults can be initialised in some init() method.
| /** | ||
| * Action factory for caching fragment payload values on Redis server. Can be initialized with a configuration: | ||
| * <pre> | ||
| * productDetails { |
There was a problem hiding this comment.
productDetails ? Do you plan to migrate to api-gateway/caching.
| * cache.ttl - in seconds, default value: 60 | ||
| * </pre> | ||
| */ | ||
| public class RedisCacheActionFactory implements ActionFactory { |
There was a problem hiding this comment.
Hmm... what about @Cacheable. Please check when action's instance is created: io.knotx.fragments.handler.action.ActionProvider#isCacheable. In my opinion the current solution does not work.
| public void apply(FragmentContext fragmentContext, Handler<AsyncResult<FragmentResult>> resultHandler) { | ||
| String cacheKey = getCacheKey(fragmentContext.getClientRequest()); | ||
|
|
||
| redisClient.connect(onConnect -> { |
There was a problem hiding this comment.
We create the new connection each time the action is invoked. Can we reuse the connection? What about reconnecting on error?
See:
3 participants
Description
Implement redis cache action example
Motivation and Context
This example was missing
Types of changes
Checklist:
I hereby agree to the terms of the Knot.x Contributor License Agreement.