Feature/mutable options

[holomekc]

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

[vietj]

I would prefer for now to not split options between mutable and not mutable (i.e keep the same options)

[holomekc]

Hi @vietj
So you say instead I should use the same object twice but sometimes directly if needed and sometimes via Supplier<Future<...>>?

[vietj]

@holomekc yes

[holomekc]

I was forced to add GenIgnore here. The generator could not handle the Supplier<Future<>> construct. Not sure if there is another way.

[vietj]

you can try using instead @GenIgnore(GenIgnore.PERMITTED_TYPE) instead, we should support this better in the future indeed

[holomekc]

Hi @vietj
I think I applied the requested changes. Please take another look.

Thank you and have a great day
Chris

[vietj]

@holomekc I will continue the review of this sorry for the delay

[vietj]

can you rebase on latest master ? I can see a conflict with the RedisClusterConnection @holomekc

[vietj]

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.

[holomekc]

Ok. Two questions:

1. 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 :)
2. Do you want that I also remove the options from all the client constructor? Or use the resolved options mentioned from question 1?

[vietj]

1. why do we need an extra options that we need to resolve ?
2. we need to keep backward compatibility, so the simple options, should be wrapped as a supplier that always returns the same options

[holomekc]

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.

[vietj]

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.

[vietj]

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).

[vietj]

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.

[vietj]

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.

[holomekc]

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

[vietj]

Overall it looks good, a couple of comments to address before continuing the review

[vietj]

@holomekc I refactored the RedisOptions in this PR #395 to decompose them in PoolOptions, NetClientOptions and RedisConnectOptions

we should use this to support a per client type method with a supplier, e.g

• RedisClient createStandaloneClient(Vertx vertx, NetClientOptions tcpOptions, PoolOptions poolOptions, Supplier<RedisConnectOptions> supplier)
• RedisClient createSentinelClient(Vertx vertx, NetClientOptions tcpOptions, PoolOptions poolOptions, Supplier<RedisSentinelConnectOptions> supplier)
• etc...

[holomekc]

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.