Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] match_only_text type migration - Stage 1 #1415

Merged
merged 25 commits into from
Jul 15, 2021

Conversation

ebeahan
Copy link
Member

@ebeahan ebeahan commented May 13, 2021

Summary

Continuing onto Stage 1 proposal for migrating text type fields, including the message base field, to use the match_only_text data type being introduced in Elasticsearch 7.14.

Stage 1 (Draft) Criteria:

  • Opened pull request for this proposal revising the existing strawperson
  • Identified "sponsor" at Elastic who will participate in RFC process and take ownership of the change after the process completes
  • Outlined initial field definitions
  • High-level description of examples of usage
  • High-level description of example sources of data
  • Identified potential concerns and implementation challenges/complexity
  • Subject matter experts identified and weighed in on the high level utility of these changes in the pull request
  • ECS team weighed in on appropriateness of these changes in the pull request
  • Assess potential performance impact

Preview of markdown proposal

@ebeahan ebeahan added the RFC label May 13, 2021
@ebeahan ebeahan self-assigned this May 13, 2021
@ebeahan ebeahan marked this pull request as ready for review May 14, 2021 15:01
@ebeahan ebeahan requested a review from a team May 14, 2021 15:37
@ebeahan
Copy link
Member Author

ebeahan commented May 14, 2021

Notable changes for stage 1:

  • I've extended the proposal also to include the current text multi-fields. I am happy to discuss more here if there are concerns or feedback, but I think we should pursue migrating to match_only_text across all of ECS as a replacement for text. For the majority of ECS use cases, users would likely benefit from the reduced storage use and not be impacted by the limitations of `match_only_text
  • Listed the ECS team has a sponsor here. Since this would be a broad change across various field sets, I think it makes sense for ECS to sponsor the proposal. Again, open to discussion.
  • Added detail to the Scope of Impact, Source data, and Concerns sections.

I'd appreciate it if reviewers would review and comment on:

  • The list of candidate fields to migrate. Concerns with migrating all text fields?
  • Any concerns with ECS team self-sponsoring?
  • Any missing concerns or considerations?

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@djptek djptek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to add actionable item for performance impact analysis on both the two original fields as well as the 49 multi-fields added in Stage 1

@djptek
Copy link
Contributor

djptek commented May 24, 2021

sent a merge so the checks would pass

@ebeahan
Copy link
Member Author

ebeahan commented May 25, 2021

After some out-of-band discussion on performance testing these changes, I'm removing the .text multi-fields from this RFC. This will narrow the scope of the proposed change to two text fields: message and error.message.

Migrating the .text multi-fields will be proposed in a future RFC (and perhaps multiple future RFCs).

Copy link
Contributor

@djptek djptek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ebeahan
Copy link
Member Author

ebeahan commented Jun 3, 2021

Holding off on merging until potential performance impact is assessed.

@ebeahan ebeahan merged commit e946986 into elastic:master Jul 15, 2021
@ebeahan ebeahan deleted the rfc/0023/stage-1 branch July 15, 2021 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants