-
Notifications
You must be signed in to change notification settings - Fork 2
[DT-2541] Support Elasticsearch 5 and OpenSearch 3 simultaneously #2765
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
Conversation
| public Response searchDatasetIndexStream(@Auth DuosUser duosUser, String query) { | ||
| try { | ||
| InputStream inputStream = elasticSearchService.searchDatasetsStream(query); | ||
| InputStream inputStream = openSearchService.searchDatasetsStream(query); |
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.
We use openSearch for the V2 endpoint.
These are mainly preexisting issues. |
kevinmarete
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.
Looks good, a few nit comments!
src/main/java/org/broadinstitute/consent/http/service/DatasetService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/broadinstitute/consent/http/service/DatasetService.java
Outdated
Show resolved
Hide resolved
71d01fa to
fd9a56d
Compare
|
eweitz
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.
Looks good!
Any work implied by my comments strikes me as reasonable to consider ticketing for later. Looking forward to more performant Data Library filtering.
src/main/java/org/broadinstitute/consent/http/service/DatasetRegistrationService.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/consent/http/service/OpenSearchService.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/consent/http/service/OpenSearchService.java
Show resolved
Hide resolved
src/main/java/org/broadinstitute/consent/http/service/OpenSearchService.java
Show resolved
Hide resolved
| openSearch: | ||
| servers: | ||
| - localhost | ||
| indexName: iName | ||
| datasetIndexName: dataset | ||
| port: 9201 |
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.
We'll need a corresponding helmfile config update to support this: https://github.com/broadinstitute/terra-helmfile/blob/master/charts/consent/templates/_consent.yaml.tpl
| @NotNull private String indexName; | ||
|
|
||
| @NotNull private List<String> servers; | ||
|
|
||
| @NotNull private String datasetIndexName; |
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.
If we are staging the OpenSearch rollout over time and we don't have values for production, this will likely cause the app to fail on startup.



Addresses
https://broadworkbench.atlassian.net/browse/DT-2541
Summary
This replaces the PR in #2755 . This allows OpenSearch 3 and Elasticsearch 5 to be supported simultaneously. I duplicated the Elasticsearch resources and then cleaned up the code . The following section is required in the
docker-compose.yml:Have you read CONTRIBUTING.md lately? If not, do that first.