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

Add support for OpenSearch #6131

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Add support for OpenSearch #6131

merged 2 commits into from
Nov 5, 2024

Conversation

stweil
Copy link
Member

@stweil stweil commented Jul 18, 2024

This PR adds support for OpenSearch while still working with Elasticsearch.

It is for people who want to work with recent code, newer OpenJDK versions (17, 22, but 11 also still supported) and OpenSearch because they cannot use Elasticsearch.

It's also for people who want to use Elasticsearch with newer code, either for security reasons or because they want to work with newer OpenJDK versions.

@stweil
Copy link
Member Author

stweil commented Jul 18, 2024

The latest code can be built with up to OpenJDK 22.

@solth
Copy link
Member

solth commented Jul 19, 2024

@stweil thanks for sharing this development. It is good to see that compatibilty with OpenSearch can be achieved this way. However, the current plan for Kitodo.Production is not to switch from ElasticSearch to the open source fork OpenSearch, but rather to incorporate the HibernateSearch framework to allow institutions to decide which search server variant they want to use (see #5760 and #6065 for details).

Of course your development is of value anyway, but I do not think creating a pull request with these changes is the right way of giving users access to it and it might trigger wrong expections (even if its just a draft), because - as you said - it won't be merged. As mentioned above, replacing the ElasticSearch client libraries in Kitodo.Production with corresponding OpenSearch libraries is currently not on the roadmap. Instead I would suggest to link the branch that this pull request is based (stweil:opensearch) in the discussion and issue mentioned above so that users interested in trying it out can access it that way.

@stweil stweil force-pushed the opensearch branch 8 times, most recently from 2c05db6 to 6e4cc69 Compare July 26, 2024 10:48
@stweil stweil force-pushed the opensearch branch 6 times, most recently from 689cbb9 to c5c9a3b Compare August 3, 2024 03:01
@stweil stweil force-pushed the opensearch branch 2 times, most recently from 7a51655 to 0911f31 Compare August 9, 2024 16:30
@stweil stweil force-pushed the opensearch branch 2 times, most recently from c41844b to 2289209 Compare August 30, 2024 15:10
@stweil stweil marked this pull request as ready for review September 3, 2024 08:16
@stweil
Copy link
Member Author

stweil commented Sep 3, 2024

I rebased and minimized the pull request now. Based on PR #6215 there remains a single small commit.

With this pull request it is directly possible to use Elasticsearch as before (see CI).

The PR replaces old Elasticsearch module by newer OpenSearch modules, so OpenSearch can also be used instead of Elasticsearch (tested locally with the same commands as in CI test, but an OpenSearch server).

It's also possible to rebase the existing work for HibernateSearch on top of this PR with only a few merge conflicts which are easy to fix.

@stweil stweil changed the title Add support for Opensearch Add support for OpenSearch Sep 3, 2024
@stweil stweil force-pushed the opensearch branch 3 times, most recently from 3e0febd to 7b9bb11 Compare October 8, 2024 15:14
@solth
Copy link
Member

solth commented Oct 8, 2024

@stweil I just tested your branch and can confirm that it works direktly "out-of-the-box" with my existing ElasticSearch-Server, without the need for any other adjustments. Additionally, it seems all changes in the code - apart from the new dependencies in the pom files - amount to just the elasticsearch import statements being replaced with corresponding opensearch imports, withtout any changes to the usage in the actual business logic at all. I have to admit I am suprissed that facilitating OpenSearch compatibility while maintaining ElasticSearch compatibility was possible with so few changes.

That said, I do not see the advantage of having these changes in the code when they are replaced by HibernateSearch before the next larger release, e.g. 3.8. Therefore I think merging this pull request would only make sense if we shifted the HibernateSearch integration to a new release 3.9 and released a version 3.8 of Kitodo in the near future with the OpenSearch compatibility and the new functions for the metadata editor for which pull requests have already been opened.

Since the HibernateSearch integration has the highest priority right now, I wouldn't want to introduce any changes to the code that would impede that project, so I would like @matthias-ronge decide whether he agrees with these changes or whether he thinks they would further postpone the finalization of the HibernateSearch integration for him.

@stweil
Copy link
Member Author

stweil commented Oct 9, 2024

Recent changes (0e43651, 1f6a523) caused a merge conflict which is now fixed.

@solth solth added this to the Kitodo.Production 3.8.0 milestone Oct 25, 2024
pom.xml Outdated Show resolved Hide resolved
@stweil stweil force-pushed the opensearch branch 2 times, most recently from 741e0dc to 9a4f8e1 Compare October 28, 2024 16:13
@stweil
Copy link
Member Author

stweil commented Oct 28, 2024

I had to rebase the PR and fix a merge conflict caused by commit 8f2015b.

It's now again ready for merging.

@stweil
Copy link
Member Author

stweil commented Oct 31, 2024

Now rebased and fixed new merge conflict.

The change was prepared with these commands:
``
perl -pi -e s/org.elasticsearch/org.opensearch/g $(git grep -l org.elasticsearch)
perl -pi -e s/ElasticsearchStatusException/OpenSearchStatusException/g $(git grep -l ElasticsearchStatusException)
```

Signed-off-by: Stefan Weil <sw@weilnetz.de>
@stweil
Copy link
Member Author

stweil commented Nov 5, 2024

Rebased and fixed merge conflict.

Copy link
Member

@solth solth left a comment

Choose a reason for hiding this comment

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

Just tested this branch with ElasticSearch and OpenSearch servers and it indeed works with both as advertised 👍

@solth solth merged commit d5189df into kitodo:master Nov 5, 2024
4 checks passed
@stweil stweil deleted the opensearch branch November 5, 2024 11:00
@solth solth modified the milestone: Kitodo.Production 3.8.0 Nov 15, 2024
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.

4 participants