Implement a Redis backed version of the command-router service #3621
Conversation
…span and redis). The infinispan and redis modules produce two separate command-router images.
StFS
requested review from
calohmn,
dejanb,
kaniyan and
sophokles73
as code owners
| <dependency> | ||
| <groupId>io.vertx</groupId> | ||
| <artifactId>vertx-web</artifactId> | ||
| <artifactId>vertx-core</artifactId> |
There was a problem hiding this comment.
I removed a dependency on vertx-web and replaced it with vertx-core which does not seem to have had any adverse effects. So my feeling is that vertx-web was not needed and vertx-core is sufficient.
| </dependency> | ||
| --> | ||
|
|
||
| <!-- testing --> |
There was a problem hiding this comment.
Currently there are no unit tests in this module. If none are added, these dependencies should be removed. I need to dive into this a bit and see if I can figure out some way to set up mock tests for the redis cache.
| import io.vertx.redis.client.Response; | ||
|
|
||
| /** | ||
| * TODO. |
There was a problem hiding this comment.
Still a lot of TODOs in this class and log lines that will be removed.
| @Override | ||
| public Future<JsonObject> checkForCacheAvailability() { | ||
| LOG.info("VREDIS: checkForCacheAvailability()"); | ||
| Objects.requireNonNull(api); |
There was a problem hiding this comment.
It probably doesn't make any sense to have this line in this method (and the ones below)? It's probably enough to have them in the static initializers and constructors.
There was a problem hiding this comment.
I'm not really sure what this file is doing exactly for the infinispan module. I kept it in here as a reminder to see if something similar is needed for the redis module.
I'm hoping that the fact that we're using the quarkus-redis-client extension will mean that this file is not needed for the redis implementation.
| * The Quarkus based Command Router main application class (Redis variant). | ||
| */ | ||
| @ApplicationScoped | ||
| public class Application extends AbstractApplication { |
There was a problem hiding this comment.
As previously mentioned. This class is exactly the same as the one in command-router-infinispan. Not sure if it's possible to simplify this even further.
There was a problem hiding this comment.
Not sure if this is needed for the redis implementation. Don't really know what this file does.
There was a problem hiding this comment.
Not sure if this file needs to exist for the redis implementation. If it should exist, I assume we'd want to have it include redis config instead of infinispan config.
There was a problem hiding this comment.
Not sure if this file is needed.
| <artifactId>hono-service-command-router-base</artifactId> | ||
| <version>${project.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.hono</groupId> | ||
| <artifactId>hono-service-command-router-infinispan</artifactId> | ||
| <version>${project.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.eclipse.hono</groupId> | ||
| <artifactId>hono-service-command-router-redis</artifactId> | ||
| <version>${project.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
I created two "concrete" modules here that will produce docker images: hono-service-command-router-infinispan and hono-service-command-router-redis.
The problem with that is that we'd be changing the legacy name of the command router service (hono-service-command-router). So the question is whether I should instead use the legacy name for the Infinispan implementation, in order to maintain some sort of a backwards compatibility? It feels "cleaner" to me to explicitly name the docker images with the technology that they support and that's the way it's done in the device-registry service (one is explicitly named -mongodb and the other one is -jdbc). But that could potentially cause some trouble where the old image name suddenly stops being produced.
Another solution to this could be to have the pipeline that builds and publishes the images tag the -infinispan image additionally with the old name as well?
|
Closing this PR since I'll split this into two PRs instead, one for splitting up the maven modules and another one for adding the redis based command router. |
Addresses #3532
Split
services/command-routermodule into three modules (-base,-infinispanand-redis). Theinfinispanandredismodules produce two separate command-router docker images.Almost all of the command-router application logic is located in the
command-router-basemodule while the-redisand-infinispanmodules only contain the main entrypoint to the application and the configuration of theDeviceConnectionInfoinstance to be backed by redis or infinispan respectively.The
client-device-connectionmodule has also been split up intoclient-device-connection,client-device-connection-infinispanandclient-device-connection-redis.A new profile has been added to the integration tests project,
redis_cache, which starts up a redis container and configures the command-router service to use the redis backed cache.All integration tests run as well as all unit tests that existed before this change.
There are some TODOs (mostly adding JavaDoc comments) and there might be room to add some unit tests to the new modules. There is also no documentation added as part of this PR yet.
Also, I'm not sure how to add configuration for running the integration tests using the Redis backed
command-routerservice so that has not been done but should probably be. The same goes for native image creation. That has not been done either.