Close query cache on index service creation failure#48230
Close query cache on index service creation failure#48230DaveCTurner merged 3 commits intoelastic:masterfrom
Conversation
Today it is possible that we create the `QueryCache` and then fail to create the owning `IndexService` and this means we do not close the `QueryCache` again. This commit addresses that leak. Fixes elastic#48186
|
Pinging @elastic/es-distributed (:Distributed/CRUD) |
jimczi
left a comment
There was a problem hiding this comment.
It looks like we can have another leak here ? The index analyzers are created by the IndexService so the build could move to the IndexModule ? These constructors are also not supposed to throw exceptions so it should also be possible to delay the IllegalArgumentException to MapperService#merge ? The exception should be removed in master since it is not applicable anymore so we probably don't need to spend too much time on it but I wonder if the model is safe here since we cannot protect all constructors with closeable object against random runtime exceptions ?
jimczi
left a comment
There was a problem hiding this comment.
LGTM
I'm wondering about moving the creation of MapperService/IndexFieldDataService/BitsetFilterCache/IndexCache out as well, even though I don't think these leak meaningfully. Makes this change quite a bit larger and adds a bunch more constructor parameters. WDYT?
Agreed, maybe add a comment explaining why we expect an exception here and we can clean up when we remove the IllegalArgumentException in MapperService on master ?
|
@jimczi Are you sure this exception handling stops being necessary when the obvious |
I am not sure, maybe this is the right protection but I wonder if throwing a runtime exception in a constructor is a bug or a feature and if all constructors that handle closeable objects should take care of these exceptions that are not declared. I am maybe overthinking this but we had the case in another constructor recently so I am fine keeping the protection if this is something that we do everywhere ? |
Today it is possible that we create the `QueryCache` and then fail to create the owning `IndexService` and this means we do not close the `QueryCache` again. This commit addresses that leak. Fixes #48186
The previous commit, 3a6fa0b introduces a compile error that was fixed locally but not committed. This commit adds the missing change.
Today it is possible that we create the `QueryCache` and then fail to create the owning `IndexService` and this means we do not close the `QueryCache` again. This commit addresses that leak. Fixes #48186
Today it is possible that we create the `QueryCache` and then fail to create the owning `IndexService` and this means we do not close the `QueryCache` again. This commit addresses that leak. Fixes elastic#48186
Today it is possible that we create the
QueryCacheand then fail to createthe owning
IndexServiceand this means we do not close theQueryCacheagain. This commit addresses that leak.
Fixes #48186