Preserve grok pattern ordering and add sort option #61671
Conversation
danhermann
added
>enhancement
:Data Management/Ingest Node
|
Pinging @elastic/es-core-features (:Core/Features/Ingest) |
elasticmachine
added
the
Team:Data Management
|
@elasticmachine update branch |
|
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
There was a problem hiding this comment.
Thanks @danhermann for adding this, I left one serialization question and small possible improvement
|
|
||
| Request(StreamInput in) throws IOException { | ||
| super(in); | ||
| this.sorted = in.readBoolean(); |
There was a problem hiding this comment.
Should this be guarded with version check? What will happen if we receive request from older node where there where no sorted field?
| } catch (Exception e) { | ||
| listener.onFailure(e); | ||
| } | ||
| } | ||
|
|
||
| static Map<String, String> getGrokPatternsResponse(Map<String, String> patterns, boolean sorted) { | ||
| return sorted ? new TreeMap<>(patterns) : patterns; |
There was a problem hiding this comment.
We could extract this new TreeMap<>(patterns) to a field to only sort all patterns once as they are constant. We could then skip this method completely.
There was a problem hiding this comment.
I shuffled around a few things in 9d3a31a to sort the patterns only once while retaining the ability to test the sort option. If you think it's useful, the sorted patterns member could be pulled into a singleton enum or the like for lazy loading. I was on the fence as to whether that would be particularly beneficial.
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
The grok processor REST endpoint returns the bundled grok patterns in an undetermined order. This PR changes the default behavior to return them in the order in which they were read from disk which preserves the natural grouping of related patterns such as "S3_ACCESS_LOG" and "ELB_URI" for AWS-related patterns though they share no common prefix. An option to return all patterns sorted by key was also added.
Resolves #40819.