-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix the "highlight.max_analyzer_offset" request parameter with "plain" highlighter #10919
Fix the "highlight.max_analyzer_offset" request parameter with "plain" highlighter #10919
Conversation
Compatibility status:Checks if related components are compatible with change e469e62 Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/performance-analyzer.git] |
Gradle Check (Jenkins) Run Completed with:
|
Thanks @drunken-monkey! Looks like it failed in https://build.ci.opensearch.org/job/gradle-check/28988/. You're one step closer to a fix ;) Did it fail as expected? Aka "The length of [field1] field of [1] doc of [test1] index has exceeded [10] - maximum allowed to be analyzed for highlighting ....". |
@dblock Yes, it failed as expected. So you’re right, good first step. |
This PR is stalled because it has been open for 30 days with no activity. |
Hi @drunken-monkey, Is this being worked upon? Pls free to reach out to maintainers for further reviews. |
As written above, I’m completely new to this project and doubt I’d be able to provide an actual fix. Adding the test was easy enough, since it was just one self-explanatory YAML file, but I’d have no idea where to start for a fix. So, no, not being worked on, would be great if someone else would find the time. |
❌ Gradle check result for 12adef4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
I think I managed to resolve this problem. Please review! The previous test failure was this:
However, when setting |
❌ Gradle check result for 48b1dcd: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
48b1dcd
to
1529dfd
Compare
@andrross Done. |
❌ Gradle check result for 1529dfd: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #10919 +/- ##
============================================
- Coverage 71.45% 71.41% -0.05%
- Complexity 59831 59835 +4
============================================
Files 4959 4959
Lines 281129 281135 +6
Branches 40857 40859 +2
============================================
- Hits 200892 200771 -121
- Misses 63580 63730 +150
+ Partials 16657 16634 -23 ☔ View full report in Codecov by Sentry. |
…h plain highlighter. Signed-off-by: Thomas Seidl <remus@gmx.net>
…ghter. Signed-off-by: Thomas Seidl <remus@gmx.net>
Signed-off-by: Thomas Seidl <remus@gmx.net>
Signed-off-by: Thomas Seidl <remus@gmx.net>
640f8b5
to
e469e62
Compare
@drunken-monkey Apologies for the delay and thanks for sticking with this issue. The PR looks good to me. I just rebased and fixed the conflicts. |
❕ Gradle check result for e469e62: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-10919-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5d327a2f48f4c0fcd8a443fa6675dad8b18b5cb5
# Push it to GitHub
git push --set-upstream origin backport/backport-10919-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x Then, create a pull request where the |
…" highlighter (opensearch-project#10919) (cherry picked from commit 5d327a2)
…" highlighter (opensearch-project#10919) (cherry picked from commit 5d327a2) Signed-off-by: Andrew Ross <andrross@amazon.com>
…" highlighter (opensearch-project#10919) (cherry picked from commit 5d327a2)
…ter with "plain… (#12357) * Fix the "highlight.max_analyzer_offset" request parameter with "plain" highlighter (#10919) (cherry picked from commit 5d327a2) Signed-off-by: Andrew Ross <andrross@amazon.com> * Change bwc test to properly reflect version Signed-off-by: Andrew Ross <andrross@amazon.com> --------- Signed-off-by: Andrew Ross <andrross@amazon.com> Co-authored-by: Thomas Seidl <drunken-monkey@users.noreply.github.com>
…" highlighter (opensearch-project#10919) Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Description
Currently, the
highlight.max_analyzer_offset
query parameter is ignored when using theplain
highlighter.Note: At the moment, this includes only a failing test, no actual fix for the problem.
Related Issues
Resolves #10702.
Check List
New functionality includes testing.New functionality has been documented.New functionality has javadoc addedPublic documentation issue/PR createdBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.