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

Avoid UUID.randomUUID() in Verticle deployment startup code #5469

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 31, 2025

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

As requested in #5450 (comment)

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
@vietj
Copy link
Member

vietj commented Jan 31, 2025

I think for this one we might have a deploymend ID in the deployment options to force a specific ID like we have done for file system path, this provides full control over the ID to quarkus

@geoand
Copy link
Contributor Author

geoand commented Feb 1, 2025

Makes sense. I'll update the PR next week

@geoand
Copy link
Contributor Author

geoand commented Feb 3, 2025

Now that I think of it, wouldn't it make more sense for DeploymentOptions to use a Supplier<String> which would be called by the DeploymentManager for each verticle?

@vietj
Copy link
Member

vietj commented Feb 4, 2025

Now that I think of it, wouldn't it make more sense for DeploymentOptions to use a Supplier<String> which would be called by the DeploymentManager for each verticle?

actually no, with vertx 5 we want to that Options object are purely data (json) if we need something with a supplier we would go with a builder instead

@vietj vietj modified the milestones: 5.1.0, 5.0.0 Feb 4, 2025
@geoand
Copy link
Contributor Author

geoand commented Feb 4, 2025

What builder are you talking about?

@tsegismont
Copy link
Contributor

tsegismont commented Feb 4, 2025

@geoand I believe @vietj is talking about the effort we have made in Vert.x 5 to have data objects, well, pure data objects and use builders when non data config is required (see for example vert-x3/vertx-micrometer-metrics#229)

Anyway, we don't need anything from DeploymentOptions, it's really the DeploymentManager that has to assign a unique ID to each deployment.

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@tsegismont tsegismont requested a review from vietj February 4, 2025 13:49
@geoand
Copy link
Contributor Author

geoand commented Feb 4, 2025

🙏🏽

@vietj
Copy link
Member

vietj commented Feb 4, 2025

@geoand sorry for the lack of context :-), indeed @tsegismont gave the context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants