-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Switch Vert.x, datasource, reactive datasource and Agroal to @ConfigMapping #33228
Conversation
...s/agroal/runtime/src/main/java/io/quarkus/agroal/runtime/DataSourcesJdbcBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
cfbae00
to
1fc9e5c
Compare
This comment has been minimized.
This comment has been minimized.
Please, have a look at smallrye/smallrye-config#939. It would be great if we could try this properly before merging and releasing it in SR Config, so we can make any adjustments. |
1fc9e5c
to
edda14a
Compare
5d8c153
to
8796e3a
Compare
@radcortez I have been struggling with this for a while and I think I will need your help. Unfortunately, I have two issues converting this extension:
I added some comments in the code, I will point them to you in subsequent GitHub comments. I will start to test your Map default branch now and report back. |
// TODO @radcortez, this apparently returns a String now instead of a List<String> | ||
var scripts = config.loadScript(); |
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.
This is the @WithConverter
issue that leads to a class cast exception.
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 need to map it like this:
Optional<List<@WithConverter(TrimmedStringConverter.class)String>> loadScript();
@WithConverter
now applies in the exact nested element type.
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.
That's what I did and the class cast error is gone:
@ConfigDocDefault("import.redis in DEV, TEST ; no-file otherwise")
Optional<List<@WithConverter(TrimmedStringConverter.class) String>> loadScript();
But it looks like the converter is somehow not applied:
Caused by: io.quarkus.runtime.configuration.ConfigurationException: Unable to find file referenced in 'quarkus.redis.redis-load-script=import/my-import.redis, sample.redis'. Remove property or add file to your path.
at io.quarkus.redis.deployment.client.RedisClientProcessor.preloadRedisData(RedisClientProcessor.java:263)
at io.quarkus.redis.deployment.client.RedisClientProcessor.init(RedisClientProcessor.java:185)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:909)
at io.quarkus.builder.BuildContext.run(BuildContext.java:282)
at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
at java.base/java.lang.Thread.run(Thread.java:829)
at org.jboss.threads.JBossThread.run(JBossThread.java:501)
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.
It needs to be defined as:
Optional<@WithConverter(TrimmedStringConverter.class) List<String>> loadScript();
Sorry for misleading in the previous commented. Just pushed a fix.
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.
Mkay. TBH, that's odd, I would have expected the converter to be placed on its target rather than the container.
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.
Yes, it is a little misleading. I'll need to improve that.
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.
|
||
tcp.localAddress().ifPresent(net::setLocalAddress); | ||
tcp.nonProxyHosts().ifPresent(net::setNonProxyHosts); | ||
// TODO @radcortez proxyOptions() used to be empty when nothing was set and is not empty anymore |
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.
This is where the problem with Optional
manifests itself.
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.
The issue here is that it can work both ways. You may want an instance of the configuration with the defaults and use it, or completely disregard it. This was not introduced with the defaults for Maps work.
Looking into the mapping and business logic, what we are saying is:
"Use these defaults only if certain properties are defined"
This creates conditional elements where only certain things are set. This was indeed supported when the default was only set with @WithDefault
, but caused issues when we wanted to override only the default (since in that case it would trigger the element creation)
We simplified this, by creating elements for defaults in every case, and it is up to the user to apply the business logic on how they want to handle it. You are going to find cases where you want always to find the defaults, and there are going to be cases where you only want to use them if other properties are set (like the Proxy case here).
I believe a more correct mapping to represent such case is something like:
@ConfigMapping(prefix = "proxy")
interface ProxyOptionalDefaults {
ProxyConfig proxy();
interface ProxyConfig {
@WithParentName
Optional<ProxyHost> host();
@WithParentName
ProxySettings settings();
}
interface ProxyHost {
String host();
}
interface ProxySettings {
@WithDefault("3128")
int port();
}
}
I'll have a look. |
@radcortez BTW, maybe I was unclear but the rest is working. The two remaining issues are the ones I mentioned about the Redis extension. |
476cc16
to
8796e3a
Compare
No, I've got it :) I've already replied to both. |
71350ad
to
718a5aa
Compare
718a5aa
to
3549078
Compare
🏆 |
You did all the work. I've just fixed my own bugs :) |
This comment has been minimized.
This comment has been minimized.
@radcortez unfortunately, I lost your commit when I rebased and I remember it was just a line but couldn't figure it out. Could you again push your fix on top of the rebased branch? Sorry and thanks! |
Sure. Done! |
Hum, now sure why but my commit is only showing in your branch and the PR did not update automatically. |
That's very odd. Let me try to get the commit, rebase and force push. |
b6ed878
to
a6a94d4
Compare
It worked, let's see what CI has to say. |
This comment has been minimized.
This comment has been minimized.
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.
LGTM overall, though I'd be lying if I said this was a pleasant read :x
Also, I added a few minor comments below.
core/runtime/src/main/java/io/quarkus/runtime/annotations/ConfigDocIgnore.java
Outdated
Show resolved
Hide resolved
...nsions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java
Outdated
Show resolved
Hide resolved
@ConfigItem | ||
public int reconnectAttempts; | ||
@WithDefault("0") | ||
int reconnectAttempts(); |
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.
Is there a reason for this new @WithDefault("0")
? Isn't 0
the default for primitive integers anyway?
Just asking in case this is a workaround for a weird behavior I should be aware of...
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.
See my answer above. For booleans, SmallRye Config was complaining so I made them explicit.
@ConfigItem | ||
public boolean tcpKeepAlive; | ||
@WithDefault("false") | ||
boolean tcpKeepAlive(); |
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.
Same here, I'd expect false
to be the default for primitive booleans... ?
@ConfigItem | ||
public boolean trustAll; | ||
@WithDefault("false") | ||
boolean trustAll(); |
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.
Same here, I'd expect false
to be the default for primitive booleans... ?
...tasource/runtime/src/main/java/io/quarkus/datasource/runtime/DataSourcesBuildTimeConfig.java
Outdated
Show resolved
Hide resolved
@ConfigItem | ||
public DataSourceJdbcRuntimeConfig jdbc; | ||
DataSourceJdbcRuntimeConfig jdbc(); | ||
|
||
/** | ||
* Additional named datasources. | ||
*/ | ||
@ConfigDocSection | ||
@ConfigDocMapKey("datasource-name") | ||
@ConfigItem(name = ConfigItem.PARENT) | ||
public Map<String, DataSourceJdbcOuterNamedRuntimeConfig> namedDataSources; | ||
@WithParentName | ||
@WithDefaults | ||
Map<String, DataSourceJdbcOuterNamedRuntimeConfig> namedDataSources(); |
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.
Any reason not to merge these into a single datasources()
method?
If so, can you please add a comment here so I don't break your stuff next time I end up in this part of the code? :)
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 it doesn't fly because you have:
quarkus.datasource.jdbc....
quarkus.datasource."name".jdbc....
I didn't really try but I thought it would be hard to make it work.
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 I suppose we'll send a PR about that later
...src/main/java/io/quarkus/datasource/deployment/spi/DevServicesDatasourceResultBuildItem.java
Outdated
Show resolved
Hide resolved
...nt-spi/src/main/java/io/quarkus/datasource/deployment/spi/DevServicesDatasourceProvider.java
Outdated
Show resolved
Hide resolved
DevServicesConfig devServicesConfig = new DevServicesConfig() { | ||
|
||
@Override | ||
public boolean enabled() { | ||
return enabled; | ||
} | ||
|
||
@Override | ||
public Optional<String> imageName() { | ||
return Optional.empty(); | ||
} | ||
|
||
@Override | ||
public OptionalInt port() { | ||
return OptionalInt.empty(); | ||
} | ||
|
||
@Override | ||
public boolean shared() { | ||
return false; | ||
} | ||
|
||
@Override | ||
public String serviceName() { | ||
return null; | ||
} | ||
|
||
@Override | ||
public Map<String, String> containerEnv() { | ||
return Map.of(); | ||
} | ||
}; | ||
|
||
DevServiceConfiguration devServiceConfiguration = new DevServiceConfiguration() { | ||
|
||
@Override | ||
public DevServicesConfig devservices() { | ||
return devServicesConfig; | ||
} | ||
}; | ||
|
||
return devServiceConfiguration; |
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.
Same here, I'd suggest mockito.
This unifies the config reference doc and make sure we have both runtime and build time config in the same file.
64a3552
to
e244bbe
Compare
- Class CrashingXADataSource used methods[1], which were added in 3.3.0 [1] quarkusio/quarkus#33228
/cc @geoand @radcortez @yrodiere