Conversation
| <dependency> | ||
| <groupId>com.github.JensPiegsa</groupId> | ||
| <artifactId>wiremock-extension</artifactId> | ||
| <version>0.3.1</version> |
There was a problem hiding this comment.
Version is better to be extracted to a property
| val bodyMono = request.bodyToMono(RequestObject.class) | ||
| .doOnSuccess(requestObject -> | ||
| sendRequestToSecondaryPrebidCacheHosts(requestObject, request.queryParam(SECONDARY_CACHE_KEY) | ||
| .orElse(Strings.EMPTY))); |
There was a problem hiding this comment.
Please user StringUtils.EMPTY from apache-commons instead
| val bodyMono = request.bodyToMono(RequestObject.class); | ||
|
|
||
| val bodyMono = request.bodyToMono(RequestObject.class) | ||
| .doOnSuccess(requestObject -> |
There was a problem hiding this comment.
In my opinion this might be not the right place to perform replication, because at this point UUIDs for cache entries are not generated yet
| private int timeoutMs; | ||
| private long minExpiry; | ||
| private long maxExpiry; | ||
| private List<String> secondaryIps; |
There was a problem hiding this comment.
Proposed configuration approach seems a little too restraining. For example cache servers might run on different ports - it's impossible to utilize such setup with current configuration.
The most simple and straightforward approach might work better - have a list of URLs where requests need to be propagated, i.e. List<String> secondaryUris (assuming these URIs point to the root context of cache application)
| if(!secondaryCache.equals("yes")) { | ||
| config.getSecondaryIps().forEach(ip -> { | ||
| WebClient.create().post().uri(uriBuilder -> uriBuilder.scheme(config.getSecondaryCacheScheme()) | ||
| .host(ip).port(config.getSecondaryCachePort()).path(CACHE_PATH) |
There was a problem hiding this comment.
It's dangerous to hard-code "/cache" into the path as cache application might be exposed via reverse-proxy. It's better to move that part into configuration.
| private void sendRequestToSecondaryPrebidCacheHosts(RequestObject requestObject, String secondaryCache) { | ||
| if(!secondaryCache.equals("yes")) { | ||
| config.getSecondaryIps().forEach(ip -> { | ||
| WebClient.create().post().uri(uriBuilder -> uriBuilder.scheme(config.getSecondaryCacheScheme()) |
There was a problem hiding this comment.
Since URIs don't change from call to call, maybe they can be cached?
| XML("pbc.${prefix}.xml"), | ||
| ERROR_DB("pbc.${prefix}.err.db"); | ||
| ERROR_DB("pbc.${prefix}.err.db"), | ||
| SECONDARY_CACHE_WRITE_ERROR("error.secondary.cache.write"); |
There was a problem hiding this comment.
This one should follow convention for enum values and metric names: ERROR_SECONDARY_WRITE("pbc.err.secondaryWrite") or alike
|
|
||
| import com.google.common.collect.ImmutableMap; | ||
| import org.apache.logging.log4j.util.Strings; | ||
| import org.luaj.vm2.ast.Str; |
There was a problem hiding this comment.
I'm pretty sure this import is not used.
| private void sendRequestToSecondaryPrebidCacheHosts(RequestObject requestObject, String secondaryCache) { | ||
| if(!secondaryCache.equals("yes")) { | ||
| config.getSecondaryIps().forEach(ip -> { | ||
| WebClient.create().post().uri(uriBuilder -> uriBuilder.scheme(config.getSecondaryCacheScheme()) |
There was a problem hiding this comment.
Does it make sense to create WebClient only once, not for every request?
3 participants
added secondary cache replication functionality
incoming request copies exactly one time to each other cache server described in config file, without retries in case of failure