[FLINK-30702] Add Elasticsearch dialect #67

grzegorz8 · 2023-07-03T07:21:33Z

https://issues.apache.org/jira/browse/FLINK-30702

grzegorz8 · 2023-08-17T06:51:22Z

@MartijnVisser Hi! I've fixed code formatting, so I think build can be rerun now. Thanks!

grzegorz8 · 2023-11-10T09:28:09Z

@eskabetxe Could you review the PR, please?

eskabetxe

Hi @grzegorz8, thanks for your contribution.
I left some comments

but should be this PR on https://github.com/apache/flink-connector-elasticsearch ??

eskabetxe · 2023-11-17T17:14:46Z

...org/apache/flink/connector/jdbc/testutils/databases/elasticsearch/ElasticsearchMetadata.java

+    public ElasticsearchMetadata(ElasticsearchContainer container) {
+        this.containerHost = container.getHost();
+        this.containerPort = container.getMappedPort(ELASTIC_PORT);
+        this.username = USERNAME;


can we obtain this from container?

I'm afraid not. We can only extract password from container envs (ELASTIC_PASSWORD).

eskabetxe · 2023-11-17T17:16:23Z

...org/apache/flink/connector/jdbc/testutils/databases/elasticsearch/ElasticsearchDatabase.java

+        CONTAINER.start();
+
+        // JDBC plugin is available only in Platinum and Enterprise licenses or in trial.
+        if (!getClient().trialEnabled()) {


@MartijnVisser can you validate this? I dont know if by license we can do this.

eskabetxe · 2023-11-17T17:19:33Z

...a/org/apache/flink/connector/jdbc/testutils/databases/elasticsearch/ElasticsearchImages.java

+/** Elasticsearch docker images. */
+public interface ElasticsearchImages {
+
+    String ELASTICSEARCH_8 = "docker.elastic.co/elasticsearch/elasticsearch:8.8.1";


any reason for that specific version? current on 8.11.1

8.8.1 was the latest one when I created the PR.
Version updated.

eskabetxe · 2023-11-17T17:21:24Z

...g/apache/flink/connector/jdbc/testutils/databases/elasticsearch/ElasticsearchRestClient.java

+                && response.license.type.equals("trial");
+    }
+
+    public void enableTrial() throws Exception {


@MartijnVisser here comes the real action

eskabetxe · 2023-11-17T17:23:13Z

flink-connector-jdbc/pom.xml

 		<postgres.version>42.5.1</postgres.version>
 		<oracle.version>21.8.0.0</oracle.version>
 		<trino.version>418</trino.version>
 		<byte-buddy.version>1.12.10</byte-buddy.version>
+		<elasticsearch.version>8.8.1</elasticsearch.version>


why this version? last version is 8.11.1

Version updated.

eskabetxe · 2023-11-17T17:24:54Z

...va/org/apache/flink/connector/jdbc/databases/elasticsearch/dialect/ElasticsearchDialect.java

+    }
+
+    @Override
+    public Optional<String> getUpsertStatement(


if read is the only option for ES, this should be on documentation

Do you mean docs/content/docs/connectors/table/jdbc.md? Which section is the most suitable one?

MartijnVisser · 2023-11-20T12:35:04Z

flink-connector-jdbc/pom.xml

 		<postgres.version>42.5.1</postgres.version>
 		<oracle.version>21.8.0.0</oracle.version>
 		<trino.version>418</trino.version>
 		<byte-buddy.version>1.12.10</byte-buddy.version>
+		<elasticsearch.version>8.11.1</elasticsearch.version>


We need to be really careful about what's included here, since the Elasticsearch dependency that's being references isn't an Apache-compatible license

Code that is licensed solely under the Elastic License 2.0 is found only in the x-pack folder.
https://github.com/elastic/elasticsearch/blob/main/LICENSE.txt

To be honest I'm not fully aware of all consequences of mixing licences here.

Below I summarize all elastic dependencies which are used:

org.elasticsearch.plugin:x-pack-sql-jdbc - Elastic License 2.0 - The dependency has provided scope so it will not be part of the connector "fat-jar". If someone wants to use it, has to add it to classpath/container/etc on their own. Does it make us "safe"?

org.elasticsearch.client:elasticsearch-rest-client (https://mvnrepository.com/artifact/org.elasticsearch.client/elasticsearch-rest-client) used with test scope - Apache 2 License - we are safe here.

@MartijnVisser Hi! What should be done to ensure if adding this dependency (even as "provided") is allowed? The same question for enabling Elastic trial for testing purposes.
Should I create a ticket somewhere? Write to [email protected]? What do you suggest? Or should we already close the PR and corresponding ticket due to incompatible license?

The license is not listed on Category X: https://www.apache.org/legal/resolved.html#category-x (nor in any other). Assuming Elastic License is "Category X", I think we are still safe, since we do not violate the following rules mentioned in the document: "They may not be distributed" and "You may rely on them when they support an optional feature".

What should be done to ensure if adding this dependency (even as "provided") is allowed?

I think we should add an enforcer rule to make sure that it's never bundled. Next to that, I think we should make a similar statement in the docs as we've done for RabbitMQ (see https://nightlies.apache.org/flink/flink-docs-master/docs/connectors/datastream/rabbitmq/)

@MartijnVisser Both enforcer rule and statement about JDBC driver license have been added.

grzegorz8 · 2023-11-21T16:25:51Z

Hi @grzegorz8, thanks for your contribution. I left some comments

but should be this PR on https://github.com/apache/flink-connector-elasticsearch ??

I just thought flink-connector-jdbc is the most suitable place - I merely add a JDBC dialect. This repo already contains lots of test utils that are used for elastic dialect as well.

Alternatively, we can add a new module flink-jdbc-dialect (or similar name) to flink-connector-elasticsearch but I'm not sure it is the best idea.

grzegorz8 · 2024-08-06T09:01:51Z

Due to recent refactoring it was easier to create a new PR: #136
Closing this one.

boring-cyborg bot added component=Connectors/JDBC component=Documentation labels Jul 3, 2023

grzegorz8 mentioned this pull request Jul 3, 2023

[FLINK-30702] Introduce ElasticDialect #16

Closed

grzegorz8 force-pushed the FLINK-30702-attempt2 branch from 9d951cf to 68c4dda Compare November 10, 2023 08:08

eskabetxe reviewed Nov 17, 2023

View reviewed changes

MartijnVisser reviewed Nov 20, 2023

View reviewed changes

grzegorz8 force-pushed the FLINK-30702-attempt2 branch from 14d5978 to c742808 Compare January 3, 2024 10:16

grzegorz8 requested a review from MartijnVisser February 2, 2024 07:53

grzegorz8 added 4 commits April 4, 2024 08:38

[FLINK-30702] Add Elasticsearch dialect

6032746

Add Elastic JDBC Driver licence statement

5899ebb

Add enforcer rule for elastic dependencies

e48944c

Update elastic x-pack-sql-jdbc version

ce985b9

grzegorz8 force-pushed the FLINK-30702-attempt2 branch from c742808 to ce985b9 Compare April 4, 2024 08:01

grzegorz8 mentioned this pull request Aug 6, 2024

[FLINK-30702] Add Elasticsearch dialect #136

Open

grzegorz8 closed this Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-30702] Add Elasticsearch dialect #67

[FLINK-30702] Add Elasticsearch dialect #67

grzegorz8 commented Jul 3, 2023

grzegorz8 commented Aug 17, 2023

grzegorz8 commented Nov 10, 2023

eskabetxe left a comment •

edited

Loading

eskabetxe Nov 17, 2023

grzegorz8 Nov 20, 2023

eskabetxe Nov 17, 2023

eskabetxe Nov 17, 2023

grzegorz8 Nov 20, 2023

eskabetxe Nov 17, 2023

eskabetxe Nov 17, 2023

grzegorz8 Nov 20, 2023

eskabetxe Nov 17, 2023

grzegorz8 Nov 20, 2023

MartijnVisser Nov 20, 2023 •

edited

Loading

grzegorz8 Nov 21, 2023

grzegorz8 Dec 8, 2023 •

edited

Loading

MartijnVisser Dec 28, 2023

grzegorz8 Jan 3, 2024

grzegorz8 commented Nov 21, 2023

grzegorz8 commented Aug 6, 2024

[FLINK-30702] Add Elasticsearch dialect #67

[FLINK-30702] Add Elasticsearch dialect #67

Conversation

grzegorz8 commented Jul 3, 2023

grzegorz8 commented Aug 17, 2023

grzegorz8 commented Nov 10, 2023

eskabetxe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartijnVisser Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzegorz8 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzegorz8 commented Nov 21, 2023

grzegorz8 commented Aug 6, 2024

eskabetxe left a comment •

edited

Loading

MartijnVisser Nov 20, 2023 •

edited

Loading

grzegorz8 Dec 8, 2023 •

edited

Loading