Skip to content

Conversation

sabino
Copy link

@sabino sabino commented Jul 12, 2025

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2025

CLA assistant check
All committers have signed the CLA.

@sabino sabino force-pushed the feat/opensearch-connector branch from a4ddcb5 to fd83e49 Compare July 12, 2025 03:17
@@ -63,6 +63,24 @@ jobs:
MINIO_API_PORT_NUMBER: 9999
AWS_EC2_METADATA_DISABLED: true
MINIO_DEFAULT_BUCKETS: peerdb
opensearch:
image: opensearchproject/opensearch:2.13.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
image: opensearchproject/opensearch:2.13.0
image: opensearchproject/opensearch:3

@serprex serprex requested a review from heavycrystal July 12, 2025 13:24
Copy link
Contributor

@heavycrystal heavycrystal left a comment

Choose a reason for hiding this comment

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

At a high-level, it seems to me that the connector is a direct adaptation of the ES connector with a different library. Is there differences besides the library where having an entirely separate connector is needed?

If not, it seems that OpenSearch can be a "flavour" of the ES connector (similar to how MySQL/MariaDB works). Code should switch on what library to use underneath via a shim or similar

@sabino
Copy link
Author

sabino commented Jul 22, 2025

@heavycrystal yes! Thanks for the review.

You're absolutely right that this shares a lot of ground with the existing Elasticsearch connector. I even brought this up in the Community Slack before implementing it, as my initial thought was also to shim the client based on the backend selecting OpenSearch or Elasticsearch dynamically.

At the moment, I don't have strong opinions either way. That said, my reasoning for keeping them separate for now is that it helps clarify and isolate the nuances between the two. Especially as their APIs and behaviors are already diverging in subtle ways. For example, the client libraries differ, and certain API endpoints and auth mechanisms are evolving independently.

That said, OpenSearch still claims compatibility with Elasticsearch 7.10 clients, so it might indeed make sense to unify them into a single connector for now. Using a client shim and perhaps probing the server version or exposing a configuration flag to let the user choose.

Happy to revisit the structure based on your thoughts.

@heavycrystal
Copy link
Contributor

@heavycrystal yes! Thanks for the review.

You're absolutely right that this shares a lot of ground with the existing Elasticsearch connector. I even brought this up in the Community Slack before implementing it, as my initial thought was also to shim the client based on the backend selecting OpenSearch or Elasticsearch dynamically.

At the moment, I don't have strong opinions either way. That said, my reasoning for keeping them separate for now is that it helps clarify and isolate the nuances between the two. Especially as their APIs and behaviors are already diverging in subtle ways. For example, the client libraries differ, and certain API endpoints and auth mechanisms are evolving independently.

That said, OpenSearch still claims compatibility with Elasticsearch 7.10 clients, so it might indeed make sense to unify them into a single connector for now. Using a client shim and perhaps probing the server version or exposing a configuration flag to let the user choose.

Happy to revisit the structure based on your thoughts.

I think it would be easier to maintain if it was a single connector with shims and switches for different behaviour. I don't anticipate drastic changes happening, and we can always revisit this if they do. You can take a look at the work we did for MySQL/MariaDB for example, there's quite a few differences but it's still a single connector.

You should be able to reuse the same tests and just test both flavours, let me know if this works

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