Skip to content
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

Use a composite loader to inject Quarkus properties into the driver configuration #75

Open
olim7t opened this issue Jun 15, 2020 · 3 comments
Assignees
Labels
priority:major Imported from JIRA status:open Imported from JIRA type:improvement Imported from JIRA

Comments

@olim7t
Copy link

olim7t commented Jun 15, 2020

Currently the extension uses the programmatic config loader to inject the options coming from the Quarkus config (CassandraClientConfig and CassandraClientConnectionConfig).

The problem is that this doesn't allow users to bring their own config loader: if they want to customize low-level options via the native driver config, it has to be an application.conf file in the classpath, it's not possible to use other built-in loader (e.g. DriverConfigLoader.fromUrl), or a custom loader.

Driver 4.7.0 introduces DriverConfigLoader.compose() to solve this:

  • use a map-based config loader for the Quarkus options:
OptionsMap quarkusConfig = new OptionsMap();

    // copy all the options of CassandraClientConfig, etc, into the map
    ```* override methods in `QuarkusCqlSessionBuilder` to always use the Quarkus options first:
```java
@NonNull
public QuarkusCqlSessionBuilder withConfigLoader(@Nullable DriverConfigLoader configLoader) {
return super.withConfigLoader(overrideWithQuarkusConfig(configLoader));
}

@nonnull
@deprecated
protected DriverConfigLoader defaultConfigLoader() {
return overrideWithQuarkusConfig(super.defaultConfigLoader());
}


@nonnull
protected DriverConfigLoader defaultConfigLoader(@nullable ClassLoader classLoader) {
return overrideWithQuarkusConfig(super.defaultConfigLoader(classLoader));
}


    private DriverConfigLoader overrideWithQuarkusConfig(DriverConfigLoader configLoader) {
      return DriverConfigLoader.compose(
          DriverConfigLoader.fromMap(quarkusConfig), // takes precedence
          configLoader);
    }
    ```* we should also add a hook to allow the user to inject their own config loader. I'm not sure of the best way to do that in Quarkus, maybe make it injectable into the session producer bean?

PS: don't use `OptionsMap.driverDefaults()` to initialize the Quarkus config, we want it to only contain the options explicitly declared by Quarkus.



┆Issue is synchronized with this [Jira Task](https://datastax-oss.atlassian.net/browse/QUAR-11) by [Unito](https://www.unito.io)
@adutra
Copy link
Contributor

adutra commented Jun 23, 2020

How are system properties overrides handled in such a composition:

return DriverConfigLoader.compose(
      DriverConfigLoader.fromMap(quarkusConfig), // takes precedence
      configLoader);

It seems that Quarkus config will have precedence over system properties.

@adutra adutra added this to the 1.0.0-alpha2 milestone Jun 23, 2020
@adutra adutra added priority:major Imported from JIRA status:open Imported from JIRA type:improvement Imported from JIRA labels Jun 23, 2020
@adutra adutra removed this from the 1.0.0-alpha2 milestone Jul 2, 2020
@tomekl007 tomekl007 self-assigned this Sep 1, 2020
@tomekl007
Copy link
Contributor

tomekl007 commented Sep 1, 2020

@olim7 I see a couple of drawbacks:

  1. The current mechanism is using both files
    ConfigFactory.parseResources("application.conf").withFallback(ConfigFactory.parseResources("application.json"))
    as the fallback and right now it is a part of the public API (and client contract) so we still must support it.
    So this means that we would need to create OptionsMap but also still use the ProgrammaticDriverConfigLoaderBuilder.
    see https://github.com/datastax/cassandra-quarkus/pull/130/files#diff-535ef3e00080110cf5c9d6d562874ff1R70-R71

  2. The problem that @adutra mentioned here:
    Use a composite loader to inject Quarkus properties into the driver configuration #75 (comment)
    we need the system properties to have the priority over quarkusConfig.

  3. I am getting some weird DriverLoader exception when using OptionsMap API:



java.lang.ClassCastException: java.util.AbstractMap$SimpleEntry cannot be cast to java.lang.Comparable

	at com.datastax.oss.driver.shaded.guava.common.collect.NaturalOrdering.compare(NaturalOrdering.java:26)
	at java.util.TimSort.countRunAndMakeAscending(TimSort.java:355)
	at java.util.TimSort.sort(TimSort.java:234)
	at java.util.Arrays.sort(Arrays.java:1512)
	at com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSortedSet.construct(ImmutableSortedSet.java:368)
	at com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSortedSet.copyOf(ImmutableSortedSet.java:304)
	at com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSortedSet.copyOf(ImmutableSortedSet.java:324)
	at com.datastax.oss.driver.shaded.guava.common.collect.ImmutableSortedSet.copyOf(ImmutableSortedSet.java:245)
	at com.datastax.oss.driver.internal.core.config.composite.CompositeDriverExecutionProfile.entrySet(CompositeDriverExecutionProfile.java:199)
	at com.datastax.oss.driver.api.core.config.DriverExecutionProfile.getComparisonKey(DriverExecutionProfile.java:202)
	at com.datastax.oss.driver.internal.core.util.Reflection.buildFromConfigProfiles(Reflection.java:153)
	at com.datastax.oss.driver.internal.core.context.DefaultDriverContext.buildLoadBalancingPolicies(DefaultDriverContext.java:340)
	at com.datastax.oss.driver.internal.core.util.concurrent.LazyReference.get(LazyReference.java:55)
	at com.datastax.oss.driver.internal.core.context.DefaultDriverContext.getLoadBalancingPolicies(DefaultDriverContext.java:725)
	at com.datastax.oss.driver.internal.core.session.DefaultSession$SingleThreaded.init(DefaultSession.java:338)
	at com.datastax.oss.driver.internal.core.session.DefaultSession$SingleThreaded.access$1100(DefaultSession.java:300)
	at com.datastax.oss.driver.internal.core.session.DefaultSession.lambda$init$0(DefaultSession.java:146)

(this is happening on this code:
https://github.com/datastax/cassandra-quarkus/pull/130/files#diff-535ef3e00080110cf5c9d6d562874ff1R70-R71)

regarding this:

if they want to customize low-level options via the native driver config, it has to be an application.conf file in the classpath, it's not possible to use other built-in loader (e.g. DriverConfigLoader.fromUrl), or a custom loader.

I think that this behavior could be achieved a little bit simpler, by allowing clients to override this method with a custom logic:

private ProgrammaticDriverConfigLoaderBuilder createDriverConfigLoaderBuilder() {
    return new DefaultProgrammaticDriverConfigLoaderBuilder(
        () ->
            // The fallback supplier specified here is similar to the default
            // one, except that we don't accept application.properties
            // because it's used by Quarkus.
            ConfigFactory.parseResources("application.conf")
                .withFallback(ConfigFactory.parseResources("application.json"))
                .withFallback(ConfigFactory.defaultReference(CqlSession.class.getClassLoader())),
        DefaultDriverConfigLoader.DEFAULT_ROOT_PATH) {
      @NonNull
      @Override
      public DriverConfigLoader build() {
        return new NonReloadableDriverConfigLoader(super.build());
      }
    };
  }

The build() method can return DriverConfigLoader.fromUrl or similar allowing clients to parameterize this behavior.
This way we don't need to use OptionsMap API. I think that it is offering enough flexibility for clients.

wdyt?

@adutra adutra added this to the 1.0.0-beta1 milestone Oct 2, 2020
@adutra
Copy link
Contributor

adutra commented Oct 3, 2020

I am getting some weird DriverLoader exception when using OptionsMap API:

Due to JAVA-2887: apache/cassandra-java-driver#1505.

We will have to wait for driver 4.10 to be able to switch to composite profiles.

@adutra adutra removed this from the 1.0.0-beta1 milestone Feb 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:major Imported from JIRA status:open Imported from JIRA type:improvement Imported from JIRA
Projects
None yet
Development

No branches or pull requests

3 participants