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

Updating the plugin to OpenSearch 2.0 and Querqy-Lucene 5.4 #18

Merged
merged 9 commits into from
Sep 13, 2022

Conversation

ps48
Copy link
Collaborator

@ps48 ps48 commented Jul 1, 2022

Upgrades include:

  1. OpenSearch bump to 2.0
  2. Querqy-Lucene 5.4
  3. Gradle 7.4.2
  4. Updated CI workflow to match OpenSearch Core and Java versions

NOTE: I see 3 test failures, @JohannesDaniel @renekrie might need some help with these.
- querqy.opensearch.RewriterShardContextsTest.testClearRewritersFromCache -> Seems to be flaky
- querqy.opensearch.rewriter.WordBreakCompoundRewriterFactoryTest.testCreateRewriter -> Maybe related to #11
- querqy.opensearch.rewriter.WordBreakCompoundRewriterFactoryTest.testThatDefaultConfigurationIsApplied -> Maybe related to upstream querqy changes

Issues resolved: #16, #15

  • Update Gradle to version 7
  • Add upstream changes from OpenSearch 2.0
  • Add upstream changes from Querqy -Lucene 9
  • Update tests
  • Check tests and build

@ps48 ps48 requested review from JohannesDaniel and renekrie July 1, 2022 16:40
@renekrie
Copy link
Contributor

renekrie commented Jul 1, 2022

"querqy.opensearch.RewriterShardContextsTest.testClearRewritersFromCache -> Seems to be flaky" - I'm seeing the same under ES. It seems that in some cases, the test frame work does not go through the process of creating the querqy query to the Querqy plugin but reuses the Querqy query object that is put together in the test. As a result, the request processor does not get injected into query object, which finally results in an NPE.

Let me push my branch to querqy-elasticsearch, maybe this will show a solution for some of the other issues.

@renekrie renekrie marked this pull request as draft July 1, 2022 17:34
@renekrie
Copy link
Contributor

renekrie commented Jul 1, 2022

I converted this into a draft PR so that we don't merge it accidentally

@renekrie
Copy link
Contributor

I finally found a fix for the flaky RewriterShardsContextsTest in the query-elasticsearch version: https://github.com/querqy/querqy-elasticsearch/pull/23/files#diff-3d9c183f14569b387180b9b3b4c237991c4ff47113d3f345fe5d41bacc2af7b0

The problem is that the test framework seems to re-use the QuerqyQueryBuilder object in case the request goes to the 'local node' instead of fully processing a request and creating a new QuerqyQueryBuilder object. In that case, the QuerqyPlugin is not invoked and cannot inject a QuerqyProcessor object, which leads to an NPE. As the nodes are picked randomly by the test framework, the test doesn't always fail.
Solution: I pick the first node to send the query to and then route the query to the other nodes via search request preferences.
I have to say that this was a bit trial and error as this is very hard to debug because of the underlying asynchronous request handling - so I'm not 100% sure of my workaround but I haven't seen a test failure for quite a bit.
@ps48 Would you have the capacity to port this to this PR?

@ps48
Copy link
Collaborator Author

ps48 commented Sep 7, 2022

@renekrie Sure I'll take this up, sorry for the delay. Missed the above comment.

@ps48
Copy link
Collaborator Author

ps48 commented Sep 12, 2022

Ported changes for RewriterShardsContext and WordbreakRewriter. All tests pass for the local build
Github CI issue: #15 (comment)
Updated OpenSearch core version in workflow. CI is successful.

@ps48 ps48 mentioned this pull request Sep 12, 2022
@ps48 ps48 marked this pull request as ready for review September 12, 2022 21:36
Copy link
Contributor

@renekrie renekrie left a comment

Choose a reason for hiding this comment

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

Thank you @ps48

@ps48 ps48 merged commit 489176a into querqy:main Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Feature] Compatibility with OpenSearch version 2.0-rc1/2.0
2 participants