add redis implementation, add support for multiple cache providers #57
Conversation
handler/core/src/main/java/io/knotx/fragments/handler/action/cache/CacheProvider.java
Outdated
Show resolved
Hide resolved
handler/core/src/main/java/io/knotx/fragments/handler/action/cache/RedisCacheProvider.java
Outdated
Show resolved
Hide resolved
handler/core/src/main/java/io/knotx/fragments/handler/action/cache/RedisCacheProvider.java
Outdated
Show resolved
Hide resolved
|
I would suggest implement unit test for CacheAction with mock Cache implementation. also two seperate tests for InMemoryCache and RedisCache. Please update also a documentation |
There was a problem hiding this comment.
Could you please provide some documentation for the new Redis cache?
Also, please update the changelog.
...r/core/src/main/java/io/knotx/fragments/handler/action/cache/InMemoryCacheActionFactory.java
Show resolved
Hide resolved
handler/core/src/main/java/io/knotx/fragments/handler/action/cache/RedisCache.java
Outdated
Show resolved
Hide resolved
|
This PR is untouched since november. Is it still valid? |
| private RedisOptions getRedisOptions(JsonObject config) { | ||
| return Optional.ofNullable(config.getJsonObject("redis")) | ||
| .map(this::toRedisOptions) | ||
| .orElseGet(RedisOptions::new); |
There was a problem hiding this comment.
Seems to be an invalid case?
|
|
||
| return new RedisOptions() | ||
| .setEndpoint(SocketAddress.inetSocketAddress(port, host)) | ||
| .setPassword(password); |
|
|
||
| private static final int DEFAULT_PORT = 6379; | ||
|
|
||
| private static final long DEFAULT_TTL = 60; |
There was a problem hiding this comment.
Better move to config object?
| public void put(String key, Object value) { | ||
| if (value instanceof JsonObject || value instanceof JsonArray || value instanceof String) { | ||
| String valueToBeCached = value.toString(); | ||
| redis.setex(key, Long.toString(ttl), valueToBeCached, response -> { |
There was a problem hiding this comment.
validate if Vert.x Redis Client ensures FIFO behaviour when creating cache entries
|
|
||
| private void callDoActionAndCache(FragmentContext fragmentContext, | ||
| Handler<AsyncResult<FragmentResult>> resultHandler, String cacheKey) { | ||
| doAction.apply(fragmentContext, asyncResult -> { |
There was a problem hiding this comment.
Change to rxified Action interface
| cache.put(cacheKey, resultPayload.getMap() | ||
| .get(payloadKey)); | ||
| } | ||
| Future.succeededFuture(fragmentResult) |
There was a problem hiding this comment.
consider whether this should be configurable
There was a problem hiding this comment.
what if doAction specifies a _fallback transition, but there is something that could be cached?
In general: what is the approach for handling decorated actions' results/transitions
| Fragment fragment = fragmentContext.getFragment(); | ||
| FragmentResult result = new FragmentResult(fragment, FragmentResult.ERROR_TRANSITION); | ||
| Future.succeededFuture(result) | ||
| .setHandler(resultHandler); |
There was a problem hiding this comment.
Consider if this should be configurable? Should cache failure always mean general failure, or perhaps just some throttling would be acceptable?
| .orElse(null); | ||
| } | ||
|
|
||
| private static Object valueToObject(String value) { |
Description
Add Redis as cache provider and refactor the module so that multiple providers are possible.
Motivation and Context
Redis is nice to have out of the box. Also with the following changes, adding a new cache provider is trivial.
Screenshots (if appropriate)
Upgrade notes (if appropriate)
Types of changes
Checklist:
I hereby agree to the terms of the Knot.x Contributor License Agreement.