-
Notifications
You must be signed in to change notification settings - Fork 642
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
Support Opensearch along with Elasticsearch integration #2850
Conversation
Signed-off-by: Andriy Redko <[email protected]>
Hi @reta, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
This is in progress right now ... |
Looks very good overall. Moving code around wouldn't make it much simpler and it doesn't warrant a whole new connector as long as the APIs are so identical. But we need something better than the |
Thank you!
Sure, I will do the change shortly, the same would apply to |
Yeah, if you don't see a better option -- try it out. |
Hi @reta, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
It sounds reasonable to me, the change is done, thanks @ennru ! |
Signed-off-by: Andriy Redko <[email protected]>
@ennru seems like all is set :-) thank you! |
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 readable and functional, and an appropriate way to support both. Thanks!
elasticsearch/src/main/scala/akka/stream/alpakka/elasticsearch/SourceSettingsBase.scala
Outdated
Show resolved
Hide resolved
elasticsearch/src/main/scala/akka/stream/alpakka/elasticsearch/SourceSettingsBase.scala
Show resolved
Hide resolved
CI complains about the formatting of some Java code. Please run |
Signed-off-by: Andriy Redko <[email protected]>
Done, thanks a lot @ennru !! |
Signed-off-by: Andriy Redko <[email protected]>
Even the Scala code isn't accepted. Please run |
Signed-off-by: Andriy Redko <[email protected]>
Sorry for that, should have run the build step locally, all formatting issues should be fixed. There are some binary compatibility issues (reported by mina checks), those are difficult to avoid since we changed the hierarchy a bit, what is the best way to deal with that, target next major release? |
Thanks @reta, |
What is lacking now are mentions of Opensearch in the docs. |
I will push the update for docs shortly, thanks @ennru ! |
Signed-off-by: Andriy Redko <[email protected]>
@ennru the documentation has been added, thank you for spending time on the change |
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.
LGTM.
Thank you for adding this and sorry for leaving it unmerged for so long. |
No problem at all, thanks a mill @ennru ! 🙇 |
Adds the support Opensearch along with Elasticsearch integration.
Background
Opensearch 1.x API is fully compatible with Elasticseach 7.x API.
Design
There are multiple ways the Opensearch support could be added along with Elasticsearch. The idea of the suggested implementation is to not break existing APIs as much as possible and at the same time to not hack any existing APIs as much as possible.
Common shared pieces:
akka.stream.alpakka.common.ApiVersion
akka.stream.alpakka.common.SourceSettings
(abstract settings overApiVersion
)akka.stream.alpakka.common.WriteSettings
(abstract settings overApiVersion
)Versions split:
akka.stream.alpakka.opensearch.ApiVersion
akka.stream.alpakka.opensearch.ApiVersion
andakka.stream.alpakka.elasticsearch.ApiVersion
implementakka.stream.alpakka.common.ApiVersion
Settings split:
akka.stream.alpakka.opensearch.OpensearchConnectionSettings
akka.stream.alpakka.opensearch.OpensearchParams
akka.stream.alpakka.opensearch.OpensearchSourceSettings
akka.stream.alpakka.opensearch.OpensearchWriteSettings
The Opensearch support is largely relies on existing Elasticsearch support in order to avoid renaming classes, duplicating the implementation or / and changing packaging structure as much as possible.
@ennru would really appreciate your opinion and guidance, we are open to any redesign which in your opinion would serve the project better in long term. For the record, we have considered multiple implementations, basically the current one is the simplest and least intrusive out of all. Another paths we have explored:
common
andopensearch
modules, extract as much as possible fromelasticsearch
intocommon
and reuse across both implementationsopensearch
module and replicate theelasticsearch
implementation (== copy/paste)Thank you.
Fixes #2846