Improves doc values format deprecation message #33576
Conversation
colings86
added
>bug
blocker
:Search/Search
|
Pinging @elastic/es-search-aggs |
| @@ -76,6 +76,7 @@ public void hitsExecute(SearchContext context, SearchHit[] hits) throws IOExcept | |||
|
|
|||
| hits = hits.clone(); // don't modify the incoming hits | |||
| Arrays.sort(hits, Comparator.comparingInt(SearchHit::docId)); | |||
| List<String> noFormatFields = new ArrayList<>(); | |||
There was a problem hiding this comment.
It is a shame to allocate this for every search request that needs to fetch in this sub-phase. The escape analysis will not be able to elide the allocation because the list escapes the method in the deprecation logger invocation. Can we collect these differently?
Also, there is a REST test that exposes this deprecation message. Would you update it to include two fields and ensure the format is correct?
There was a problem hiding this comment.
I'm not too worried about the allocation, but I have a different concern which is that we collect into a list and then log 80 lines below if this list is not empty - I'd rather like to loop twice and have everything in a single place.
There was a problem hiding this comment.
It is a shame to allocate this for every search request that needs to fetch in this sub-phase. The escape analysis will not be able to elide the allocation because the list escapes the method in the deprecation logger invocation. Can we collect these differently?
Do you have something particular in mind?
Also, there is a REST test that exposes this deprecation message. Would you update it to include two fields and ensure the format is correct?
Noted. I didn't see where the test was to start with but no you pointed it out (offline) and the PR CI highlighted it too I'll address it
| DEPRECATION_LOGGER.deprecated("There are doc-value field which are not using a format. The output will " | ||
| + "change in 7.0 when doc value fields get formatted based on mappings by default. It is recommended to pass " | ||
| + "[format={}] with a doc value field in order to opt in for the future behaviour and ease the migration to " | ||
| + "7.0: [{}]", DocValueFieldsContext.USE_DEFAULT_FORMAT, noFormatFields); |
There was a problem hiding this comment.
do we really need the outer brackets, there will be double brackets in the string because of the list, right?
|
@jpountz I pushed a commit that should address your concerns. Not sure that it addresses @jasontedor's concern though |
| List<String> noFormatFields = context.docValueFieldsContext().fields().stream().filter(f -> f.format == null).map(f -> f.field) | ||
| .collect(Collectors.toList()); | ||
| if (noFormatFields.isEmpty() == false) { | ||
| DEPRECATION_LOGGER.deprecated("There are doc-value field which are not using a format. The output will " |
This changes the deprecation message when doc values fields do not supply a format form logging a deprecation warning for each offending field individually to logging a single message which lists all offending fields Closes #33572
Also adds a test to ensure multiple deprecation warnings are collated into one message
Moves the collection of fields that don't have a format to a separate loop and moves the logging of the deprecation warning to be next to it at the expesnse of looping through the field list twice
* Improves doc values format deprecation message This changes the deprecation message when doc values fields do not supply a format form logging a deprecation warning for each offending field individually to logging a single message which lists all offending fields Closes #33572 * Updates YAML test with new deprecation message Also adds a test to ensure multiple deprecation warnings are collated into one message * Condenses collection of fields without format check Moves the collection of fields that don't have a format to a separate loop and moves the logging of the deprecation warning to be next to it at the expesnse of looping through the field list twice * fixes typo * Fixes test
* Improves doc values format deprecation message This changes the deprecation message when doc values fields do not supply a format form logging a deprecation warning for each offending field individually to logging a single message which lists all offending fields Closes #33572 * Updates YAML test with new deprecation message Also adds a test to ensure multiple deprecation warnings are collated into one message * Condenses collection of fields without format check Moves the collection of fields that don't have a format to a separate loop and moves the logging of the deprecation warning to be next to it at the expesnse of looping through the field list twice * fixes typo * Fixes test
* master: Remove debug logging in full cluster restart tests (elastic#33612) Expose CCR to the transport client (elastic#33608) Mute testIndexDeletionWhenNodeRejoins SQL: Make Literal a NamedExpression (elastic#33583) [DOCS] Adds missing built-in user information (elastic#33585) Improves doc values format deprecation message (elastic#33576)
This changes the deprecation message when doc values fields do not
supply a format form logging a deprecation warning for each offending
field individually to logging a single message which lists all
offending fields
Closes #33572