Delay creation of the next SearchPhase in executeNextPhase #116061
Conversation
Delaying the creation of the next phase to only when we actually need it makes this a lot easier to reason about and should set up further simplications. Eager creation of the next phase forced a lot of needlessly complicated safety logic around resources on us. Since we never "close" the `nextPhase` on failure all its resources need to be tracked in via `context.addReleasable`. This isn't as much of an issue with some recent refactorings leaving very little resource creation in the constructors but still, delaying things saves memory and makes reasoning about failure cases far easier.
original-brownbear
added
>non-issue
:Search Foundations/Search
elasticsearchmachine
added
v9.0.0
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Your PR seems fine, but i hate to say it; they are not accepting such changes since they rejected immediately my PR yesterday that included code performance improvement :). You can try of course but don't get shocked if they don't like the change. As for me, I don't like functional programming but again, your code is fine. |
There was a problem hiding this comment.
I think the change looks straightforward, and harmless. I would like to maybe see what simplifications this unlocks next, that you mentioned in the description of the PR. Should they be included or are they adding too much complexity? I must say that I do like how this specific change is simple and contained.
|
Thanks Luca, uff lets do this one by one :D I think I have a pretty short path here for getting rid of the whole tracking of releasable resources as part of the search context, that's massively inefficiency and noisy in terms of leak tracking. Also, we can probably get rid of a bunch of search phase implementations outright and just explicitly run the code, I'll open a PR shortly :) |
original-brownbear
added
auto-backport
💔 Backport failed
You can use sqren/backport to manually backport by running |
…16061) Delaying the creation of the next phase to only when we actually need it makes this a lot easier to reason about and should set up further simplications. Eager creation of the next phase forced a lot of needlessly complicated safety logic around resources on us. Since we never "close" the `nextPhase` on failure all its resources need to be tracked in via `context.addReleasable`. This isn't as much of an issue with some recent refactorings leaving very little resource creation in the constructors but still, delaying things saves memory and makes reasoning about failure cases far easier.
…16061) Delaying the creation of the next phase to only when we actually need it makes this a lot easier to reason about and should set up further simplications. Eager creation of the next phase forced a lot of needlessly complicated safety logic around resources on us. Since we never "close" the `nextPhase` on failure all its resources need to be tracked in via `context.addReleasable`. This isn't as much of an issue with some recent refactorings leaving very little resource creation in the constructors but still, delaying things saves memory and makes reasoning about failure cases far easier.
…117369) Delaying the creation of the next phase to only when we actually need it makes this a lot easier to reason about and should set up further simplications. Eager creation of the next phase forced a lot of needlessly complicated safety logic around resources on us. Since we never "close" the `nextPhase` on failure all its resources need to be tracked in via `context.addReleasable`. This isn't as much of an issue with some recent refactorings leaving very little resource creation in the constructors but still, delaying things saves memory and makes reasoning about failure cases far easier.
4 participants
Delaying the creation of the next phase to only when we actually need it makes this a lot easier to reason about and should set up further simplications. Eager creation of the next phase forced a lot of needlessly complicated safety logic around resources on us. Since we never "close" the
nextPhaseon failure all its resources need to be tracked in viacontext.addReleasable. This isn't as much of an issue with some recent refactoring leaving very little resource creation in the constructors but still, delaying things saves memory and makes reasoning about failure cases far easier.