Only log one types warning per bulk search request. #37446
Conversation
jtibshirani
added
>non-issue
:Search/Search
|
Pinging @elastic/es-search |
jtibshirani
changed the title
There was a problem hiding this comment.
@jtibshirani left two short comments around testing, maybe those cases are already covered, otherwise adding them would be nice. Rest looks good to me.
| deprecationLogger.deprecatedAndMaybeLog("msearch_with_types", TYPES_DEPRECATION_MESSAGE); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can this be unit tested, since a similar looking test is removed from MultiSearchTemplateRequestTests? Maybe it already is tested elsewhere, but I don't see it in this PR so just asking to check to make sure.
There was a problem hiding this comment.
Yes, this deprecation warning is tested in RestMultiSearchTemplateActionTests. The assertWarnings call in MultiSearchTemplateRequestTests wasn't there to explicitly test the deprecation, but rather to make sure the test didn't fail on unexpected warnings.
There was a problem hiding this comment.
Thanks, and sorry for bothering.
There was a problem hiding this comment.
No worries, I really appreciate your constant emphasis on tests!
| deprecationLogger.deprecatedAndMaybeLog("msearch_with_types", TYPES_DEPRECATION_MESSAGE); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same here, is there a unit test already? Otherwise is is possible to add one?
There was a problem hiding this comment.
Likewise these deprecations are covered by RestMultiSearchActionTests.
|
Looks good but I wonder if it's possible to make DeprecationLogger less expensive to call repeatedly? |
|
@markharwood I agree that ideally we'd fix the underlying issue with the slow deduplication of warning headers. Maybe we could file an issue (separate from this PR) with a description of the problem, so that the core-infra team could take a look? |
There was a problem hiding this comment.
@jtibshirani thanks for the clarification on the tests, the rest LGTM
|
FYI - I opened #37530 for the DeprecationLogger performance |
In #37411, we found that issuing multiple types deprecation warnings in a
_bulkindexing request led to a large performance regression in the nightly benchmarks. This is likely because warnings are deduplicated when added to the response header, which requires some string parsing.This PR applies the same fix to msearch and msearch template requests as we did for bulk requests, to prevent any possibility of slowdown there.