-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
[FLINK-25756] [connectors/opensearch] Dedicated Opensearch connectors #18541
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 95b34f9 (Thu Jan 27 15:31:50 UTC 2022) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
a520c43
to
e31965a
Compare
Hi @reta Thanks for the contribution! |
Hey @CrynetLogistics , thanks a lot for pointing it out. No, we have not considered the Generic Async Sink API, it seems like it was introduced at the same time this pull request was created (but it sounds like a good future improvement for sure). Thank you! |
cb9026e
to
588cb7a
Compare
@reta Thanks for your patience! We've started this week with our first external connector repo project, which is moving out the Elasticsearch connector from this repository to https://github.com/apache/flink-connector-elasticsearch I think it would be best to first get that one moved out, so we can understand the actual issues that we might run into. When that one is done, I propose to create a dedicated repo for Opensearch and move your code to that repo. What do you think? |
Sounds great, thank you @MartijnVisser ! Mind looking at #18634 before moving Elasticsearch repository (cleanup test dependencies)? Thank you! |
3498b37
to
b0ea656
Compare
f285d71
to
dd322f2
Compare
@MartijnVisser I see that https://github.com/apache/flink-connector-elasticsearch is getting filled in, do you think I could re-target the pull request to this repository (or, alternatively, new one https://github.com/apache/flink-connector-opensearch could be created)? Thank you. |
Based on the latest discussion on the mailing list, we identified that if we want to create a new connector, we need to create a small FLIP (see https://cwiki.apache.org/confluence/display/FLINK/FLIP+Connector+Template). Do you have edit rights on the wiki? If not, I can ask a PMC to take care of that for you. After a FLIP discussion and a vote, we can create the repo directly. I think that makes the most sense. What do you think? |
Thanks @MartijnVisser , sure, I will do FLIP and follow the process. I don't have write permissions for Apache Flink space to create the new page for connector, I would really appreciate your help (same ASF username as on Github), thank you! |
a9dee23
to
55fbee8
Compare
1eae18f
to
eabb85e
Compare
80e9b3a
to
6b19cf6
Compare
4bb685d
to
3e627df
Compare
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
FLIP has been accepted: https://cwiki.apache.org/confluence/display/FLINK/FLIP-243%3A+Dedicated+Opensearch+connectors |
@reta Could you sync this PR to use the same setup as https://github.com/apache/flink-connector-elasticsearch ? Especially the Parent POM setup + the CI stuff. |
Will do, thanks @MartijnVisser ! |
@MartijnVisser I am closing this one in favor of apache/flink-connector-opensearch#1 |
Signed-off-by: Andriy Redko andriy.redko@aiven.io
What is the purpose of the change
The goal of this change is to provide dedicated Opensearch connectors.
Brief change log
The implementation is largely based on the existing Elasticsearch 7 connector with a few notable changes (besides the dependencies and APIs):
HighLevelRestClient
is used)allow-insecure
has been added to suppress certificates validation for development and testing purposesThe new connector name is
opensearch
and it follows the existing conventions:Verifying this change
This change added comprehensive tests and can be verified as follows (largely ported the existing unit and integration tests for Elasticsearch 7):
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation
Huge thanks @snuyanzin for help.