Produce quarkus.uuid lazily
#45434
Produce quarkus.uuid lazily
#45434
Conversation
geoand
changed the title
quarkus.uuid lazily
|
Nice one! |
|
🙏🏽 ❤️ |
| /** | ||
| * A property that allows accessing a generated UUID. | ||
| * It generates that UUID at startup time. So it changes between two starts including in dev mode. | ||
| * <br> | ||
| * Access this generated UUID using expressions: `${quarkus.uuid}`. | ||
| */ | ||
| Optional<String> uuid(); | ||
|
|
There was a problem hiding this comment.
Does it actually change something to remove this? Asking because it's the only way to document it in the config.
There was a problem hiding this comment.
If a property is declared in a mapping (or the old root), it is always eagerly populated. It wouldn't matter if quarkus.uuid is lazy. This is always trigger the load.
There was a problem hiding this comment.
OK. I suppose we need to make sure it's properly documented elsewhere then.
There was a problem hiding this comment.
I think we need to do it manually. We don't have a way to do it in the processor. We can ignore properties for documentation, but we can't ignore properties for mapping.
| @@ -12,7 +15,38 @@ public class RuntimeConfigBuilder implements SmallRyeConfigBuilderCustomizer { | |||
| @Override | |||
| public void configBuilder(final SmallRyeConfigBuilder builder) { | |||
| new QuarkusConfigBuilderCustomizer().configBuilder(builder); | |||
| builder.withDefaultValue("quarkus.uuid", UUID.randomUUID().toString()); | |||
| builder.withSources(new ConfigSource() { | |||
There was a problem hiding this comment.
Looks interesting. If the ConfigSource are kept around at runtime, we might want to move this to a static nested class to avoid keeping the builder reference around?
That being said, better to wait for @radcortez 's feedback before polishing.
There was a problem hiding this comment.
If we want a quick solution, I'm okay with moving forward with this.
My idea was to add some sort of lazy type that can be declared in the mapping. This would allow keeping the declaration for documentation purposes and flattening sources if there are other cases.
Alternatively, we could just do this as a build step for the extensions that need it (it would have worked if it was done that way when it was first introduced). Unfortunately, we can't tell if users are now relying on it outside of the required extensions.
There was a problem hiding this comment.
If we want a quick solution, I'm okay with moving forward with this.
That's totally up to you. The reason I vote for having this now is that we usually do the performance optimization in sprints (I don't mean the Agile sense of the word) and if we don't get this in now, who knows when we'll get around to it).
There was a problem hiding this comment.
In any case, I updated the PR to use an inner class instead of an anonymous one.
There was a problem hiding this comment.
I think it's worth merging this but I also agree having a lazy type in SmallRye Config would be nice for the future and we could revisit this patch then.
There was a problem hiding this comment.
BTW, another alternative (breaking) is to totally drop this from the Config system and add a specific API for it (this is similar to the issue we have with ports).
There was a problem hiding this comment.
I thought of that, but this property seems pretty pervasive so I didn't want to cause such problems until after the next LTS :)
This is done because bootstrapping the plumbing needed by the JDK to produce a UUID value is expensive, it thus doesn't make sense to pay this cost when the property isn't actually needed
geoand
added
the
triage/waiting-for-ci
This comment has been minimized.
This comment has been minimized.
|
FWIW, the fault tolerance issue is unrelated, it popped up in other PRs. I pinged @Ladicek with the details in another PR. |
|
🙏 |
Ah, good to hear, I was going crazy thinking I caused it in another PR :) |
|
We need to be careful though as it gets the build stuck so PRs are not properly tested: anything beyong these tests is not exercised... |
This is done because bootstrapping the plumbing
needed by the JDK to produce a UUID value
is expensive, it thus doesn't make sense to
pay this cost when the property isn't actually
needed
The goal is the same as that of #33941