Skip to content

Use singleton instance for default project-id #123677

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 5, 2025
Merged

Use singleton instance for default project-id #123677

merged 6 commits into from
Mar 5, 2025

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Feb 28, 2025 •
edited
Loading

No need to create new instances for the default project-id. We can use the singleton field which should speeds up key comparsion for Map.get operations.

Relates: #123662

@ywangd
Use singleton default-project instance in deserialization
ad79b5f 
No need to create new instances for the default project-id sent across
the wire. We can use the singleton field which should speeds up
key comparsion for Map.get operations.

Relates: elastic#123662
@ywangd ywangd added >non-issue :Core/Infra/Core Core issues without another label v9.1.0 labels Feb 28, 2025
@ywangd ywangd requested review from a team and tvernum February 28, 2025 04:10
ywangd
ywangd commented Feb 28, 2025
View reviewed changes
nielsbauman
nielsbauman reviewed Feb 28, 2025
View reviewed changes
Copy link
Contributor

@nielsbauman nielsbauman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense to me. I'm not sure how big of an improvement we'll see, but I don't see a reason not to do this.

edit: I just realized this will probably also speed up map lookups because we'll be able to skip the string equality (as they're the same instance).

@tvernum
Copy link
Contributor

tvernum commented Mar 4, 2025

This seems reasonably to me. I think we should turn it into a real PR.

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Mar 5, 2025
@ywangd ywangd marked this pull request as ready for review March 5, 2025 05:07
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 5, 2025
@ywangd
Copy link
Member Author

ywangd commented Mar 5, 2025

Thanks for the feedback. I updated the PR to make ProjectId class with private constructors and ensure the default project-id is a singleton everywhere. It has many cascading changes. They are mostly trivial and most of them are arguably something we should refactor even without this PR, e.g. replacing new ProjectId(randomUUID()) with randomUniqueProjectId(). I also linked it with a serverless PR. Both are ready for review. Thanks!

@ywangd ywangd requested a review from nielsbauman March 5, 2025 05:09
@ywangd ywangd changed the title Use singleton default-project instance in deserialization Use singleton instance for default project-id Mar 5, 2025
ywangd
ywangd commented Mar 5, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/cluster/metadata/ProjectId.java
Comment on lines -23 to +24
public record ProjectId(String id) implements Writeable, ToXContent {
public class ProjectId implements Writeable, ToXContent {
Copy link
Member Author

Choose a reason for hiding this comment

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

Only this file contains real changes. Everything else is cascading.

@ywangd ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 5, 2025
nielsbauman
nielsbauman approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@nielsbauman nielsbauman left a comment

Choose a reason for hiding this comment

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

LGTM. I had an entry on my TODO list for replacing all the new Project(randomUUID()) instances, which I agree had to happen at some point. Thanks for this, Yang!

@elasticsearchmachine elasticsearchmachine merged commit 1bf6a77 into elastic:main Mar 5, 2025
17 checks passed
@ywangd ywangd deleted the use-singleton-for-default-project-id-deserialization branch March 5, 2025 07:47
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Mar 11, 2025
@ywangd @georgewallace
Use singleton instance for default project-id (elastic#123677)
e762fbd 
No need to create new instances for the default project-id. We can use
the singleton field which should speeds up key comparsion for Map.get
operations.

Relates: elastic#123662
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tvernum tvernum tvernum approved these changes

@nielsbauman nielsbauman nielsbauman approved these changes

Assignees

No one assigned

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Core Core issues without another label >non-issue serverless-linked Added by automation, don't add manually Team:Core/Infra Meta label for core/infra team v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ywangd @tvernum @elasticsearchmachine @nielsbauman