Add optimized updateSingleProject on Metadata
#129585
Conversation
For legacy reasons, `Metadata.Builder` contains a map of `ProjectMetadata.Builder`s instead of a map of `ProjectMetadata`s. While the plan is still to change that, there is a significant amount of refactoring that needs to be done before we can make that change. We have seen regressions in benchmarks due to this extra conversion of project metadata -> builder -> project metadata. We add a method that allows updating a single project without having to convert all the projects to builders first.
elasticsearchmachine
added
Team:Core/Infra
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| /** | ||
| * Updates a single project in the metadata. This offers a more performant way of updating a single project compared to the Builder. | ||
| */ | ||
| public Metadata updateSingleProject(ProjectMetadata updatedProject) { |
There was a problem hiding this comment.
I'm a little bit on the fence about whether we'll want to keep this method in the future or not. When we update Metadata.Builder to contain fully constructed ProjectMetadata objects instead of builders, the value of this method is reduced a lot - it would only skip a small bit of processing in Metadata.Builder#build. We don't need to decide right now whether we'll keep the method or not, but I'm thinking whether we should add a @FixForMultiProject so we can revisit.
There was a problem hiding this comment.
My 2¢: There's no harm in adding the annotation, provided you also add a comment clarifying why it's there. We'll be in a better position to make the decision later when we've seen how things pan out. If we decide to keep it, we can just remove the annotation then.
There was a problem hiding this comment.
I'm not super keen on the name of this method. There's nothing to indicate that it's returning a copy (or rather that it's returning a copy unless it's a noop). I realize that this is obvious to many readers will be aware that Metadata is immutable and so able to figure out that it must be doing that, but maybe not all readers will, or we'll be making them stop and think.
The other methods which do similar things seem to follow the 'wither' naming convention, so maybe withUpdatedProject? I think we should omit the 'Single' as well. It's obvious that it only updates one project. And I'd like to distinguish this from the existing (private) method updateSingleProject. That existing method has a different behaviour, it throws an exception if there are multiple projects, and so presumably the Single in its name is indicated that it only works in single-project use cases — which is not the case for this method.
There was a problem hiding this comment.
Thanks for your input, Pete! I added the annotation and I renamed the method to withUpdatedProject - all your points make a lot of sense 👍.
It seems to me that the issue is that we eagerly create the builders. Some laziness could help. I had a go at it, see https://github.com/elastic/elasticsearch/compare/main...ywangd:metadata-lazy-project-builder?expand=1 I didn't put too much effort into it. So it may have its own issues. It may also feel a bit risky in pushing such a change close to release. I am ok if we want to go with a more narrowly focused solution as proposed by this PR. |
There was a problem hiding this comment.
Mostly LGTM. I think we should include the annotation we mentioned, and (predictably) I have a comment about method naming. I'll approve to unblock you, since this is fixing a regression, but I'd appreciate it if you could address both of those before merging. Thanks!
| /** | ||
| * Updates a single project in the metadata. This offers a more performant way of updating a single project compared to the Builder. | ||
| */ | ||
| public Metadata updateSingleProject(ProjectMetadata updatedProject) { |
There was a problem hiding this comment.
My 2¢: There's no harm in adding the annotation, provided you also add a comment clarifying why it's there. We'll be in a better position to make the decision later when we've seen how things pan out. If we decide to keep it, we can just remove the annotation then.
| /** | ||
| * Updates a single project in the metadata. This offers a more performant way of updating a single project compared to the Builder. | ||
| */ | ||
| public Metadata updateSingleProject(ProjectMetadata updatedProject) { |
There was a problem hiding this comment.
I'm not super keen on the name of this method. There's nothing to indicate that it's returning a copy (or rather that it's returning a copy unless it's a noop). I realize that this is obvious to many readers will be aware that Metadata is immutable and so able to figure out that it must be doing that, but maybe not all readers will, or we'll be making them stop and think.
The other methods which do similar things seem to follow the 'wither' naming convention, so maybe withUpdatedProject? I think we should omit the 'Single' as well. It's obvious that it only updates one project. And I'd like to distinguish this from the existing (private) method updateSingleProject. That existing method has a different behaviour, it throws an exception if there are multiple projects, and so presumably the Single in its name is indicated that it only works in single-project use cases — which is not the case for this method.
| @@ -1487,6 +1523,18 @@ public Metadata copyAndUpdate(Consumer<Builder> updater) { | |||
| return builder.build(); | |||
| } | |||
|
|
|||
| public Metadata copyAndUpdateProject(ProjectId projectId, Consumer<ProjectMetadata.Builder> updater) { | |||
There was a problem hiding this comment.
Aside: I think the name of this one is okay because it's following the precedent of the existing copyAndUpdate. (And because you omitted the Single here!) I guess it's a little odd that it's not following the 'wither' convention, and IIUC it's also misleading in the sense that it won't do a copy if the updater was a noop, but I think that's okay given the precedent.
Thanks @ywangd. I see some small potential bugs, but you already disclosed that it's a first version, and I think those bugs are easy to address. I don't think I have super strong feelings about which approach we go with - both will address the regression. I think I'm slightly leaning towards the approach in this PR, as the number of changes On one hand, this PR has fewer changes, which limits added complexity and should be easier to revert in the future if we want to. On the other hand, your changes are more "native"/built-in into the |
|
I am OK to go with this PR. My main consideration is whether we can fix it without adding more workaround methods if the complexity is manageable, i.e. not "significant ", in the given time window. I see we are concerned about the risk, which is totally justified and I have no objection to go with the proposed changes and revisit it after the release. |
|
For my money, the helper-type methods in this PR add less complexity than the change to the way we represent state in @ywangd 's, though I can also see how Yang's is quite elegant. It seems like our consensus is that this PR is the least risky option, right? In which case, it gets my vote (for now). |
For legacy reasons,
Metadata.Buildercontains a map ofProjectMetadata.Builders instead of a map ofProjectMetadatas. While the plan is still to change that, there is a significant amount of refactoring that needs to be done before we can make that change.We have seen regressions in benchmarks due to this extra conversion of project metadata -> builder -> project metadata. We add a method that allows updating a single project without having to convert all the projects to builders first.