Skip to content

[FLINK-29042][Connectors/ElasticSearch] Support lookup join for es connector #39

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

Merged
merged 5 commits into from
Sep 11, 2023

Conversation

liyubin117
Copy link
Contributor

@liyubin117 liyubin117 commented Sep 23, 2022

Now es connector could only be used as a sink, but in many business scenarios, we treat es as a index database, thus we should support to make it lookupable in flink.

@boring-cyborg
Copy link

boring-cyborg bot commented Sep 23, 2022

Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)

@liyubin117
Copy link
Contributor Author

@MartijnVisser Hi. Could you please give a review? Thanks!

@MartijnVisser
Copy link
Contributor

@vahmed-hamdy Since you offered to volunteer to review an Elasticsearch PR, perhaps you could also have a look at this open PR?

@liyubin117
Copy link
Contributor Author

@MartijnVisser @vahmed-hamdy Hi, The feature has taken effect in our production environment for one more years, now we want to updrade our es connector, if the feature would be merged, it would be very nice to upgrade directly!

@MartijnVisser
Copy link
Contributor

@reta Do you have any considerations before merging this?

@reta
Copy link
Member

reta commented Sep 6, 2023

@reta Do you have any considerations before merging this?

@MartijnVisser LGTM, we would probably could bring that to OpenSearch connector as well, thank you!


@Override
public String asSummaryString() {
return "Elasticsearch-6";
Copy link
Contributor

@snuyanzin snuyanzin Sep 7, 2023

Choose a reason for hiding this comment

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

Suggested change
return "Elasticsearch-6";
return "Elasticsearch6";

There is DynamicSink with Elasticsearch6, shouldn't we follow same naming conventions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wise consideration 👍

@snuyanzin
Copy link
Contributor

thanks for the contribution
also looks good from my side
i left some minor comments

+1 for having similar things for OpenSearch

@MartijnVisser
Copy link
Contributor

@snuyanzin Will you take care of the rest of the review and merge it when you think it's OK?

@snuyanzin
Copy link
Contributor

yes, sure, will try to do it later today/tomorrow

@liyubin117
Copy link
Contributor Author

liyubin117 commented Sep 7, 2023

CI failed caused by outdated dev branch, I have rebased on the latest main branch and CI succeed.
@snuyanzin PTAL, Thanks very much!

Copy link
Contributor

@snuyanzin snuyanzin left a comment

Choose a reason for hiding this comment

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

Thanks for addressing comments

@snuyanzin snuyanzin merged commit fef5c96 into apache:main Sep 11, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 11, 2023

Awesome work, congrats on your first merged pull request!

@liyubin117
Copy link
Contributor Author

liyubin117 commented Sep 12, 2023

@snuyanzin @MartijnVisser @reta Thanks a lot for your patient work.

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.

4 participants