-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Limit the analyzed text for highlighting (#27934) #28176
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
Limit the analyzed text for highlighting (#27934) #28176
Conversation
jimczi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mayya-sharipova , I left some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change coming from another pr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimczi this is a typo noticed by someone else, I thought I would include this correction here as well. Let me know if I should not do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a pr open for this typo so we should use it rather than adding this in a pr that targets 6.x only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a custom validator to throw an exception when the value is set to 0. Otherwise this would fail the upgrade to 7 where 0 is forbidden.
bf58c2e to
47c59a5
Compare
- Introduce index level setting "index.highlight.max_analyzed_offset" to control the max number of character to be analyzed for highlighting - Make this setting to be unset by default (equal to -1) - Issue a deprecation warning if setting is unset and analysis is required on a text larger than ES v.7.x max setting (10000) - Throw IllegalArgumentException is setting is set by a user, and analysis is required on a text larger than the user's set value. Closes elastic#27517 Adding validator for index.highlight.max_analyzed_offset setting
47c59a5 to
2e9f31e
Compare
| throw new IllegalArgumentException( | ||
| "[" + MAX_ANALYZED_OFFSET_SETTING.getKey() + "] must be >= 1"); | ||
| } | ||
| this.maxAnalyzedOffset = maxAnalyzedOffset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jimczi I have added a custom validator here, as you suggested. This should guard against setting value <1. Can you please continue the reviewing when you have available time.
jimczi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
to control the max number of character to be analyzed for highlighting
on a text larger than ES v 7.x max setting (10000)
analysis is required on a text larger than the user's set value.
Closes #27517