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

Support Opensearch along with Elasticsearch integration #2850

Merged
merged 7 commits into from
May 28, 2022

Conversation

reta
Copy link
Contributor

@reta reta commented Apr 5, 2022

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:

  • Introduces new marker interface akka.stream.alpakka.common.ApiVersion
  • Introduces new abstract class akka.stream.alpakka.common.SourceSettings (abstract settings over ApiVersion)
  • Introduces new abstract class akka.stream.alpakka.common.WriteSettings (abstract settings over ApiVersion)

Versions split:

  • Introduces new enumeration akka.stream.alpakka.opensearch.ApiVersion
  • Both akka.stream.alpakka.opensearch.ApiVersion and akka.stream.alpakka.elasticsearch.ApiVersion implement akka.stream.alpakka.common.ApiVersion

Settings split:

  • new companion object akka.stream.alpakka.opensearch.OpensearchConnectionSettings
  • new companion object akka.stream.alpakka.opensearch.OpensearchParams
  • new class akka.stream.alpakka.opensearch.OpensearchSourceSettings
  • new class 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:

  • introduce dedicated common and opensearch modules, extract as much as possible from elasticsearch into common and reuse across both implementations
  • introduce opensearch module and replicate the elasticsearch implementation (== copy/paste)
    Thank you.

Fixes #2846

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@lightbend-cla-validator
Copy link

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:

https://www.lightbend.com/contribute/cla

@reta
Copy link
Contributor Author

reta commented Apr 5, 2022

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

https://www.lightbend.com/contribute/cla

This is in progress right now ...

@ennru
Copy link
Member

ennru commented Apr 6, 2022

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 akka.stream.alpakka.common package as such a package would possibly conflict with other connectors. Those things could go into the elasticsearch package e.g. as ApiVersionBase, even if that doesn't look perfect either.

@reta
Copy link
Contributor Author

reta commented Apr 6, 2022

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.

Thank you!

But we need something better than the akka.stream.alpakka.common package as such a package would possibly conflict with other connectors. Those things could go into the elasticsearch package e.g. as ApiVersionBase, even if that doesn't look perfect either.

Sure, I will do the change shortly, the same would apply to SourceSettings -> SourceSettingsBase and WriteSettings -> WriteSettingsBase if I understand properly, makes sense?

@ennru
Copy link
Member

ennru commented Apr 6, 2022

Yeah, if you don't see a better option -- try it out.

@lightbend-cla-validator
Copy link

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:

https://www.lightbend.com/contribute/cla

@reta
Copy link
Contributor Author

reta commented Apr 6, 2022

Yeah, if you don't see a better option -- try it out.

It sounds reasonable to me, the change is done, thanks @ennru !

@reta reta marked this pull request as ready for review April 6, 2022 13:36
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@reta
Copy link
Contributor Author

reta commented Apr 8, 2022

@ennru seems like all is set :-) thank you!

Copy link

@RyanSkraba RyanSkraba left a 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!

@ennru
Copy link
Member

ennru commented Apr 14, 2022

CI complains about the formatting of some Java code. Please run javafmtAll and push the reformatted code.

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@reta
Copy link
Contributor Author

reta commented Apr 14, 2022

CI complains about the formatting of some Java code. Please run javafmtAll and push the reformatted code.

Done, thanks a lot @ennru !!

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@ennru
Copy link
Member

ennru commented Apr 19, 2022

Even the Scala code isn't accepted. Please run scalafmtAll and push the updates.

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@reta
Copy link
Contributor Author

reta commented Apr 19, 2022

Even the Scala code isn't accepted. Please run scalafmtAll and push the updates.

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?

@ennru
Copy link
Member

ennru commented May 1, 2022

Thanks @reta,
I had a little time to spend on this now. I believe everything should stay in the elasticsearch package so it matches the JAR file properly. I pushed an update to do so and included the MiMa exclusions as we target Alpakka 4.0 anyway.

@ennru ennru added this to the 4.0.0 milestone May 1, 2022
@ennru
Copy link
Member

ennru commented May 1, 2022

What is lacking now are mentions of Opensearch in the docs.

@reta
Copy link
Contributor Author

reta commented May 2, 2022

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 <andriy.redko@aiven.io>
@reta
Copy link
Contributor Author

reta commented May 2, 2022

@ennru the documentation has been added, thank you for spending time on the change

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru merged commit bc2decd into akka:master May 28, 2022
@ennru
Copy link
Member

ennru commented May 28, 2022

Thank you for adding this and sorry for leaving it unmerged for so long.

@reta
Copy link
Contributor Author

reta commented May 28, 2022

Thank you for adding this and sorry for leaving it unmerged for so long.

No problem at all, thanks a mill @ennru ! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Opensearch along with Elasticsearch integration
4 participants