Refactor configuration completely #692

dmlloyd · 2019-01-31T10:00:41Z

• support multiple levels of hierarchy
• handle at augment time via reflection instead of using code generation
• distinguish build-/run-time injection items
• generate config parsing with unknown key detection
• lighten annotation processing to work better with eclipse (hopefully)
• support a wide range of configuration types
• require explicit specification of build phase for configuration groups

Co-authored-by: Alexey Loubyansky loubyansky@gmail.com
Co-authored-by: Stuart Douglas stuart.w.douglas@gmail.com

FroMage · 2019-01-31T10:18:16Z

And they say my PRs are too big ;)

dmlloyd · 2019-01-31T10:18:24Z

Well this was working. Infinispan has just come in and broken it. Repair incoming.

dmlloyd · 2019-01-31T14:18:07Z

Though I've requested a few reviews, and others are welcome to review as well, I'd prefer to merge this and fix anything else after, unless there's something catastrophic.

FroMage · 2019-01-31T14:21:05Z

+1

Sanne · 2019-01-31T15:20:44Z

I've had a failure in module shamrock-integration-test-vertx when enabling -Dnative during local testing:

[INFO] Running org.jboss.shamrock.vertx.runtime.tests.VertxProducerResourceIT
Executing /home/sanne/sources/shamrock/integration-tests/vertx/target/shamrock-integration-test-vertx-1.0.0.Alpha1-SNAPSHOT-runner
java.lang.ExceptionInInitializerError
	at com.oracle.svm.core.hub.ClassInitializationInfo.initialize(ClassInitializationInfo.java:281)
	at java.lang.Class.ensureInitialized(DynamicHub.java:375)
	at org.jboss.shamrock.deployment.steps.LoggingResourceProcessor$setupLoggingRuntimeInit1.deploy(Unknown Source)
	at org.jboss.shamrock.runner.ApplicationImpl1.doStart(Unknown Source)
	at org.jboss.shamrock.runtime.Application.start(Application.java:90)
	at org.jboss.shamrock.runtime.Application.run(Application.java:190)
	at org.jboss.shamrock.runner.GeneratedMain.main(Unknown Source)
Caused by: java.lang.IllegalArgumentException: No Converter registered for class class java.time.Duration
	at io.smallrye.config.SmallRyeConfig.getConverter(SmallRyeConfig.java:152)
	at io.smallrye.config.SmallRyeConfig.convert(SmallRyeConfig.java:132)
	at org.jboss.shamrock.runtime.generated.ConfigHelper.init:vertx(Unknown Source)
	at org.jboss.shamrock.runtime.generated.ConfigHelper.<clinit>(Unknown Source)
	at com.oracle.svm.core.hub.ClassInitializationInfo.invokeClassInitializer(ClassInitializationInfo.java:341)
	at com.oracle.svm.core.hub.ClassInitializationInfo.initialize(ClassInitializationInfo.java:261)
	... 6 more
Exception in thread "main" java.lang.RuntimeException: Failed to start shamrock
	at org.jboss.shamrock.runner.ApplicationImpl1.doStart(Unknown Source)
	at org.jboss.shamrock.runtime.Application.start(Application.java:90)
	at org.jboss.shamrock.runtime.Application.run(Application.java:190)
	at org.jboss.shamrock.runner.GeneratedMain.main(Unknown Source)
Caused by: java.lang.ExceptionInInitializerError
	at com.oracle.svm.core.hub.ClassInitializationInfo.initialize(ClassInitializationInfo.java:281)
	at java.lang.Class.ensureInitialized(DynamicHub.java:375)
	at org.jboss.shamrock.deployment.steps.LoggingResourceProcessor$setupLoggingRuntimeInit1.deploy(Unknown Source)
	... 4 more
Caused by: java.lang.IllegalArgumentException: No Converter registered for class class java.time.Duration
	at io.smallrye.config.SmallRyeConfig.getConverter(SmallRyeConfig.java:152)
	at io.smallrye.config.SmallRyeConfig.convert(SmallRyeConfig.java:132)
	at org.jboss.shamrock.runtime.generated.ConfigHelper.init:vertx(Unknown Source)
	at org.jboss.shamrock.runtime.generated.ConfigHelper.<clinit>(Unknown Source)
	at com.oracle.svm.core.hub.ClassInitializationInfo.invokeClassInitializer(ClassInitializationInfo.java:341)
	at com.oracle.svm.core.hub.ClassInitializationInfo.initialize(ClassInitializationInfo.java:261)
	... 6 more

dmlloyd · 2019-01-31T15:41:43Z

Easy fix, I suppose.

dmlloyd · 2019-01-31T15:44:51Z

It's not clear why this one isn't using an implicit converter. They cannot be disabled AFAICT.

dmlloyd · 2019-01-31T15:45:17Z

Ah of course, native image. I'll have to probe these at startup.

Sanne · 2019-01-31T15:46:38Z

considering config might change.. should we just register all known converters for reflective access?

dmlloyd · 2019-01-31T16:01:49Z

Yeah. It's going to be a little bit of a challenge though; I'll have to make a build item for it.

Sanne · 2019-01-31T16:03:58Z

extensions/agroal/runtime/src/main/java/org/jboss/shamrock/agroal/runtime/DataSourceConfig.java

-    @ConfigProperty(name = "url")
-    public String url;
+    @ConfigItem
+    public Optional<String> url;


the connection url is mandatory to define a datasource. Do we still need these to be Optional now?

For now this is the only way I could exactly replicate the previous logic, which relied on optional configuration groups which will no longer be supported.

Once this is in, I think it's OK to change this to whatever is the best fit for the extension.

One thing to note is that you are not allowed to have mandatory properties in any event. So, typically this means you have to intelligently disable your service when required configuration is not present. Making a property non-optional means you'll have to have a null/empty check.

not allowed to have mandatory properties in any event

"event" ?

So we just validate within the logic of the extensions, right?

"in any event" == "in any case/situation", sorry for the ambiguous language.

Yeah the extensions have to do validation though adding generalized validation for extension config is also on my todo list.

But the reason that mandatory properties cannot work is that our model allows extensions to be brought in by other extensions, and yet we have a mandate to start up with little or zero configuration in a useful state. Allowing mandatory configuration items will break this badly. So validation should be constrained to just verifying that values that were specified are in bounds, or should otherwise only fail as a result of something the user has done wrong (as opposed to something that happened automatically).

So a good semantic is to define the minimum property or properties that are required to enable the extension, and if none are given, that means "do not add this service", and if they're only partial, perhaps log a warning that some required properties are not present so the service is not activated. Only produce an error (stop) when there is a value explicitly specified, but it is not valid.

dmlloyd · 2019-01-31T23:09:31Z

A fix for the converter issue is coming tomorrow. We don't want to merge this yet if it's going to break CI.

• support multiple levels of hierarchy • handle at augment time via reflection instead of using code generation • distinguish build-/run-time injection items • generate config parsing with unknown key detection • lighten annotation processing to work better with eclipse (hopefully) • support a wide range of configuration types • require explicit specification of build phase for configuration groups Co-authored-by: Alexey Loubyansky <loubyansky@gmail.com> Co-authored-by: Stuart Douglas <stuart.w.douglas@gmail.com>

dmlloyd · 2019-02-01T08:37:28Z

OK everything should be solved... pending CI.

objectiser · 2019-02-01T15:42:58Z

@dmlloyd Will there be further doc updates, as they are still referring to @ConfigProperty, instead of @configitem?

Also - do the properties still come from MP-config? i.e. microprofile-config.properties

dmlloyd · 2019-02-01T21:26:43Z

Sure I"ll add a todo for the docs; I will probably largely cut & paste them from the summary emails I'm going to be writing on the plane tomorrow.

Yeah still using microprofile-config.properties for the moment.

cescoffier · 2019-02-02T08:11:07Z

@dmlloyd should we use this opportunity to change the name and location of the file?

This was referenced Jan 31, 2019

Obtain configuration for Jaeger from MP-Config #654

Merged

Jaeger settings should be in config files #637

Closed

dmlloyd requested review from stuartwdouglas and Sanne January 31, 2019 14:16

Sanne reviewed Jan 31, 2019

View reviewed changes

Sanne approved these changes Jan 31, 2019

View reviewed changes

dmlloyd merged commit 0a09e82 into quarkusio:master Feb 1, 2019

dmlloyd deleted the config-new branch February 1, 2019 09:26

gsmet added this to the 0.8.0 milestone Feb 1, 2019

dmlloyd mentioned this pull request Feb 1, 2019

Get rid of color when not supported by the terminal #677

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor configuration completely #692

Refactor configuration completely #692

dmlloyd commented Jan 31, 2019

FroMage commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

FroMage commented Jan 31, 2019

Sanne commented Jan 31, 2019 •

edited

Loading

dmlloyd commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

Sanne commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

Sanne Jan 31, 2019

dmlloyd Jan 31, 2019

dmlloyd Jan 31, 2019

Sanne Feb 1, 2019

dmlloyd Feb 1, 2019

dmlloyd commented Jan 31, 2019

dmlloyd commented Feb 1, 2019

objectiser commented Feb 1, 2019

dmlloyd commented Feb 1, 2019

cescoffier commented Feb 2, 2019

Refactor configuration completely #692

Refactor configuration completely #692

Conversation

dmlloyd commented Jan 31, 2019

FroMage commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

FroMage commented Jan 31, 2019

Sanne commented Jan 31, 2019 • edited Loading

dmlloyd commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

Sanne commented Jan 31, 2019

dmlloyd commented Jan 31, 2019

Sanne Jan 31, 2019

Choose a reason for hiding this comment

dmlloyd Jan 31, 2019

Choose a reason for hiding this comment

dmlloyd Jan 31, 2019

Choose a reason for hiding this comment

Sanne Feb 1, 2019

Choose a reason for hiding this comment

dmlloyd Feb 1, 2019

Choose a reason for hiding this comment

dmlloyd commented Jan 31, 2019

dmlloyd commented Feb 1, 2019

objectiser commented Feb 1, 2019

dmlloyd commented Feb 1, 2019

cescoffier commented Feb 2, 2019

Sanne commented Jan 31, 2019 •

edited

Loading