-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Bug fix for AnnotatedTextHighlighter #39525
Conversation
Pinging @elastic/es-search |
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 Mark, I left a couple of comments. I think this would be easier to reason about if we moved the parsing and intersection methods off the Analyzer and into the PassageFormatter and Highlighter - at the moment the Analyzer is doing lots of stuff at different times, and it's quite difficult to follow the paths through the code.
server/src/main/java/org/elasticsearch/search/fetch/FetchSubPhase.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/elasticsearch/search/fetch/subphase/highlight/AnnotatedTextHighlighter.java
Outdated
Show resolved
Hide resolved
c40a276
to
9f07b20
Compare
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, one nit but no need for another go-round
@@ -318,45 +319,12 @@ public AnnotationToken getAnnotation(int index) { | |||
// original markup form in order to inject annotations. | |||
public static final class AnnotatedHighlighterAnalyzer extends AnalyzerWrapper { | |||
private Analyzer delegate; | |||
private AnnotatedText[] annotations; | |||
public AnnotatedHighlighterAnalyzer(Analyzer delegate){ | |||
private HitContext hitContext; |
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.
Can be final
9f07b20
to
e84641c
Compare
Added thread safety by moving custom Analyzer object to per-request context rather than singleton. Added YAML test that reproduced the error. Closes elastic#39395
… into AnnotatedPassageFormatter
e84641c
to
8e58f21
Compare
* 6.7: Fix CCR HLRC docs Introduce forget follower API (elastic#39718) 6.6.2 release notes. Update release notes for 6.7.0 Add documentation for min_hash filter (elastic#39671) Unmute testIndividualActionsTimeout Unmute testFollowIndexAndCloseNode Use unwrapped cause to determine if node is closing (elastic#39723) Don’t ack if unable to remove failing replica (elastic#39584) Wipe Snapshots Before Indices in RestTests (elastic#39662) (elastic#39765) Bug fix for AnnotatedTextHighlighter (elastic#39525) Fix Snapshot BwC with Version 5.6.x (elastic#39737) Fix occasional SearchServiceTests failure (elastic#39697) Correct date in daterange-aggregation.asciidoc (elastic#39727) Add a note to purge the ingest-geoip plugin (elastic#39553)
Added thread safety to AnnotatedTextHighlighter by moving custom Analyzer object to per-request context rather than singleton.
Added YAML test that reproduced the error.
Closes #39395