-
Notifications
You must be signed in to change notification settings - Fork 115
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
Feature/mutable options #391
base: 4.x
Are you sure you want to change the base?
Conversation
…y set. I am not sure though. Test are green though.
I would prefer for now to not split options between mutable and not mutable (i.e keep the same options) |
Hi @vietj |
@holomekc yes |
…MutableRedisOptions and replace everything with RedisOptions. Minor renaming, because mutable does not make much sense anymore.
* Values like type cannot be changed. | ||
* @return the client | ||
*/ | ||
@GenIgnore |
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 was forced to add GenIgnore here. The generator could not handle the Supplier<Future<>> construct. Not sure if there is another way.
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 can try using instead @GenIgnore(GenIgnore.PERMITTED_TYPE)
instead, we should support this better in the future indeed
Hi @vietj Thank you and have a great day |
@holomekc I will continue the review of this sorry for the delay |
can you rebase on latest master ? I can see a conflict with the RedisClusterConnection @holomekc |
* @return the client | ||
*/ | ||
@GenIgnore | ||
static Redis createClient(Vertx vertx, RedisOptions options, Supplier<Future<RedisOptions>> optionsSupplier) { |
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 think here we should only use the supplier and avoid the extra option. We will simply use the first option returned by the supplier to have the fixed data. That is what we did in vertx-sql-client.
After in master we should refactor the options to split between the per connection option and the general options.
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.
Ok. Two questions:
- In the create method the type is used to determine, which client to create. This would force me to resolve the future directly in createClient. I am not that familiar with the vertx future. The only method I could find is
result()
, but when I check the implementation, then value might be null. So the only other way I could see is:optionsSupplier.get().toCompletionStage().toCompletableFuture().get()
, which looks strange :) - Do you want that I also remove the
options
from all the client constructor? Or use the resolved options mentioned from question 1?
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.
- why do we need an extra options that we need to resolve ?
- we need to keep backward compatibility, so the simple options, should be wrapped as a supplier that always returns the same options
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.
Hi @vietj . I think I did not describe my issue properly. See the potential change below. I though with you first comment you want me to remove the additional RedisOptions. When I do that I only have the Supplier<Future<>>, which I need to resolve immediately. Otherwise, I have no "type" to check against. And in the example below I do not know how to resolve a vertx future in a non-reactive block. Therefore, the approach I mentioned above.
/**
* Create a new redis client using the given client options.
* @param vertx the vertx instance
* @param options the user provided options
* @return the client
*/
static Redis createClient(Vertx vertx, RedisOptions options) {
return createClient(vertx, () -> Future.succeededFuture(new RedisOptions(options)));
}
/**
* Create a new redis client using the given client options.
*
* @param vertx the vertx instance
* @param optionsSupplier the user provided options, which can be changed dynamically.
* Values like type cannot be changed.
* @return the client
*/
@GenIgnore(GenIgnore.PERMITTED_TYPE)
static Redis createClient(Vertx vertx, Supplier<Future<RedisOptions>> optionsSupplier) {
RedisOptions options;
try {
options = optionsSupplier.get().toCompletionStage().toCompletableFuture().get();
} catch (InterruptedException | ExecutionException e) {
throw new IllegalStateException("Could not resolve initial RedisOptions.");
}
switch (options.getType()) {
case STANDALONE:
return new RedisClient(vertx, options, optionsSupplier);
case SENTINEL:
return new RedisSentinelClient(vertx, options, optionsSupplier);
case CLUSTER:
return new RedisClusterClient(vertx, options, optionsSupplier);
case REPLICATION:
return new RedisReplicationClient(vertx, options, optionsSupplier);
default:
throw new IllegalStateException("Unknown Redis Client type: " + options.getType());
}
}
The second question is related to the switch block. There I need to create the different clients based on the "type". E.g.
return new RedisClient(vertx, options, optionsSupplier);
My question here was if I should also remove the options parameter and only keep optionsSupplier.
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.
ok that makes sense to me, we need to find a way to make this work.
There are several ways to solve it, I believe the easiest way is to have the createClient method returns a future instead of the type.
We could also think instead using a lazy RedisClient interface which has a volatile Future<RedisClient>
field that is initialised when first accessed.
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.
Actually I am thinking that this option should only be possible for pooled connections and not for direct connections (for which it does not make sense).
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.
So my point is that the type specificity RedisClient/RedisSentinelClient/RedisClusterClient/... should be moved more internally which I think requires a refactor of the client for this.
We should only have internally a single client class and the subclasses of Redis
should be refactored as a connection factory hierarchy like we did in the Vert.x SQL client.
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 you can just provide the PR as is and I would take care of the refactoring if that is too much work for you.
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.
Hi @vietj ,
that would be awesome actually, because I would struggle and annoy you all the time to make sure I am not messing it up.
Thank you very much,
Chris
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.
Overall it looks good, a couple of comments to address before continuing the review
@holomekc I refactored the we should use this to support a per client type method with a supplier, e.g
|
I started from scratch in this PR (#410). Resolving the conflict was not that easy. I hope this is ok and it helps. I also fixed the copy constructor in RedisOptions. Maybe you want to fix this earlier already. |
Motivation:
As requested in #376
To be honest I am not sure if I was able to implement it in the way you guys imagined it. It was really hard for me to decide, which options should be mutable and which should not. It is already a bigger change than I though at the beginning and I am not sure if I destroyed something. The tests looked good locally but who knows ;)
There are some things of the implementation I do not really like, but it should be backwards compatible. Maybe that was not the best idea I do not know, but I am afraid that is all I can contribute here. I hope this helps at least a little bit.
Br,
Chris