-
Notifications
You must be signed in to change notification settings - Fork 94
[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
Conversation
Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html) |
@MartijnVisser Hi. Could you please give a review? Thanks! |
@vahmed-hamdy Since you offered to volunteer to review an Elasticsearch PR, perhaps you could also have a look at this open PR? |
@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! |
@reta Do you have any considerations before merging this? |
@MartijnVisser LGTM, we would probably could bring that to OpenSearch connector as well, thank you! |
...pache/flink/streaming/connectors/elasticsearch/table/ElasticsearchRowDataLookupFunction.java
Outdated
Show resolved
Hide resolved
...pache/flink/streaming/connectors/elasticsearch/table/ElasticsearchRowDataLookupFunction.java
Outdated
Show resolved
Hide resolved
...pache/flink/streaming/connectors/elasticsearch/table/ElasticsearchRowDataLookupFunction.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public String asSummaryString() { | ||
return "Elasticsearch-6"; |
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.
return "Elasticsearch-6"; | |
return "Elasticsearch6"; |
There is DynamicSink with Elasticsearch6
, shouldn't we follow same naming conventions?
Lines 197 to 199 in ce44a13
public String asSummaryString() { | |
return "Elasticsearch6"; | |
} |
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.
wise consideration 👍
...a/org/apache/flink/streaming/connectors/elasticsearch/table/Elasticsearch7DynamicSource.java
Outdated
Show resolved
Hide resolved
...a/org/apache/flink/streaming/connectors/elasticsearch/table/Elasticsearch7DynamicSource.java
Outdated
Show resolved
Hide resolved
...a/org/apache/flink/streaming/connectors/elasticsearch/table/Elasticsearch7DynamicSource.java
Outdated
Show resolved
Hide resolved
thanks for the contribution +1 for having similar things for OpenSearch |
@snuyanzin Will you take care of the rest of the review and merge it when you think it's OK? |
yes, sure, will try to do it later today/tomorrow |
CI failed caused by outdated dev branch, I have rebased on the latest main branch and CI succeed. |
4842bc3
to
8444f63
Compare
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.
Thanks for addressing comments
Awesome work, congrats on your first merged pull request! |
@snuyanzin @MartijnVisser @reta Thanks a lot for your patient work. |
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.