-
Notifications
You must be signed in to change notification settings - Fork 7
Replace Elasticsearch client #744
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
base: main
Are you sure you want to change the base?
Conversation
Also remove unused constructor
These should consistently use ObjectNode, not JsonData
src/main/java/uk/ac/cam/cl/dtg/segue/etl/ElasticSearchIndexer.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/uk/ac/cam/cl/dtg/segue/search/ElasticSearchProvider.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/uk/ac/cam/cl/dtg/segue/search/ElasticSearchProvider.java
Dismissed
Show dismissed
Hide dismissed
Properly nest the expected queries
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #744 +/- ##
==========================================
- Coverage 36.84% 36.79% -0.05%
==========================================
Files 540 540
Lines 23642 23795 +153
Branches 2825 2834 +9
==========================================
+ Hits 8711 8756 +45
- Misses 14056 14158 +102
- Partials 875 881 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
generateBoolMatchQuery is deprecated; it would be nice to remove it entirely, but for now just replace it where it is straightforward to do so
This reverts commit 10eb803.
|
|
||
| // Assert | ||
| assertEquals(expectedQuery, actualQuery); | ||
| assertEquals(expectedQuery.toString(), actualQuery.toString()); |
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.
For some reason these tests fail if the queries are compared as Queries. I don't like using toString() as a workaround, but I don't have a better solution.
| filter.must(RangeQuery.of(r -> { | ||
| r.field(fieldToFilterInstruction.getKey()); | ||
| if (dateRangeInstruction.getFromDate() != null) { | ||
| r.gte(JsonData.of(dateRangeInstruction.getFromDate().getTime())); | ||
| } | ||
| if (dateRangeInstruction.getToDate() != null) { | ||
| r.lte(JsonData.of(dateRangeInstruction.getToDate().getTime())); | ||
| } | ||
| return r; | ||
| })._toQuery()); |
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.
The structure of RangeQuery builders is temporary for while we're on the new client but still Elasticsearch v7; once we upgrade the Elasticsearch version we can use the clearer date-based syntax (see https://github.com/isaacphysics/isaac-api/blob/improvement/es-upgrade-v8/src/main/java/uk/ac/cam/cl/dtg/segue/search/ElasticSearchProvider.java#L494-L505)
Replaces the high-level REST client previously used for Elasticsearch with the new(er) Java API client. This is necessary to allow Elasticsearch to be upgraded from v7, but this PR does not actually upgrade the version.