[FLINK-22207][connectors/hive]Hive Catalog retrieve Flink Properties … #15564

hameizi · 2021-04-12T08:43:05Z

What is the purpose of the change

Fix hive catalog handle propertie error "java.lang.IllegalStateException: Duplicate key". As said in FLINK-22207 like below.

If we use hive catalog and set flink propertie eg."url",and then when we add the other propertie like start with string "flink." like "flink.url" will show we the error "java.lang.IllegalStateException: Duplicate key" , and what's worse is because of this error we can`t drop or alter this tabel more.

I found in this method , "org.apache.flink.table.catalog.hive.HiveCatalog.retrieveFlinkProperties", replace all "flink." of propertie. So , the propertie of HiveCatalog "flink.url" and "flink.flink.url" both result to "url".

Brief change log

Hive Catalog retrieve Flink Properties error

Verifying this change

Manually verified.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2021-04-12T08:46:33Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 8eaeab3 (Fri May 28 09:06:10 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

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 commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2021-04-12T08:57:33Z

CI report:

2535701 Azure: FAILURE
8eaeab3 Azure: PENDING

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

KurtYoung

Thanks for the fix, could you add a test case?

hameizi · 2021-04-12T10:50:07Z

Thanks for the fix, could you add a test case?

hello , I test the bug in sql-client
Flink SQL> alter table clickhousetableflink_append_stream set ('url'='jdbc:clickhouse://host:port/default');
[INFO] Alter table succeeded!
Flink SQL> alter table clickhousetableflink_append_stream set ('flink.url'='jdbc:clickhouse://host:port/default?socket_timeout=300000');
[INFO] Alter table succeeded!
Flink SQL> drop table clickhousetableflink_append_stream;
[ERROR] Could not execute SQL statement. Reason:
java.lang.IllegalStateException: Duplicate key jdbc:clickhouse://host:port/default

After this fix is works fine

KurtYoung · 2021-04-13T01:43:26Z

@hameizi I'm pretty sure you fixed the bug and the change also LGTM. But by adding a test case can help the project won't introduce the same bug by accident in the future.

hameizi · 2021-04-13T03:34:41Z

@KurtYoung I add test case in new commit. Please review. Thanks

KurtYoung · 2021-04-13T05:58:55Z

.../flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogTest.java

@@ -78,4 +80,17 @@ public void testCreateHiveTable() {
                prop.keySet().stream()
                        .noneMatch(k -> k.startsWith(CatalogPropertiesUtil.FLINK_PROPERTY_PREFIX)));
    }
+
+    @Test
+    public void testRetrieveFlinkProperties() throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, IllegalAccessException {


Can you choose another way to test this? Relying on private method is not a good practice.

@KurtYoung Thanks for you review, i change the test case, please review it.

KurtYoung

Please also fix the format error

KurtYoung · 2021-04-13T12:28:21Z

.../flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogTest.java

+        Map<String, String> properties = new HashMap<>(new FileSystem().path("/test_path").toProperties());
+
+        properties.put(CatalogPropertiesUtil.IS_GENERIC, String.valueOf(true));
+        properties.put("url", "jdbc:clickhouse://host:port/default");


set this property to a different value and also verify you can get this property correctly?

KurtYoung · 2021-04-13T12:28:32Z

.../flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogTest.java

+        CatalogTable source = new CatalogTableImpl(schema, properties, null);
+
+        hiveCatalog.createTable(
+                hiveObjectPath, source, false);


unnecessary wrap here

Sorry, i can't understand this

Do you mean like this?

hiveCatalog.createTable( hiveObjectPath, new CatalogTableImpl(schema, properties, null), false);

KurtYoung · 2021-04-13T12:28:45Z

.../flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogTest.java

@@ -78,4 +99,23 @@ public void testCreateHiveTable() {
                prop.keySet().stream()
                        .noneMatch(k -> k.startsWith(CatalogPropertiesUtil.FLINK_PROPERTY_PREFIX)));
    }
+
+    @Test
+    public void testRetrieveFlinkProperties() throws TableAlreadyExistException, DatabaseNotExistException, TableNotExistException {


throws Exception is enough

hameizi · 2021-04-13T13:05:38Z

@KurtYoung Hello, i push a new commit, thanks for your review.

KurtYoung · 2021-04-13T13:46:09Z

.../flink-connector-hive/src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogTest.java

+                hiveObjectPath, new CatalogTableImpl(schema, properties, null), false);
+
+        CatalogBaseTable hiveTable = hiveCatalog.getTable(hiveObjectPath);
+        assertEquals(hiveTable.getOptions().get("flink.url"), "jdbc:clickhouse://host:port/testUrl2");


check we can get "url" correctly

KurtYoung · 2021-04-14T01:43:16Z

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.4.2:check (spotless-check) on project flink-connector-hive_2.11: The following files had format violations:
[ERROR] src/test/java/org/apache/flink/table/catalog/hive/HiveCatalogTest.java

…error

hameizi · 2021-04-14T05:49:33Z

@KurtYoung I fix in the new commit, please review thanks

KurtYoung

LGTM

KurtYoung · 2021-04-14T06:33:47Z

@hameizi If you want to fix this in 1.12.3, you can also create a PR against release-1.12 branch.

hameizi · 2021-04-14T06:37:29Z

@hameizi If you want to fix this in 1.12.3, you can also create a PR against release-1.12 branch.

ok, thanks

rmetzger added the review=description? label Apr 12, 2021

rmetzger added the component=Connectors/Hive label Apr 12, 2021

KurtYoung reviewed Apr 12, 2021

View reviewed changes

hameizi force-pushed the master branch from cd5ab56 to 48a157f Compare April 13, 2021 05:46

KurtYoung reviewed Apr 13, 2021

View reviewed changes

hameizi force-pushed the master branch from 48a157f to a55cd02 Compare April 13, 2021 06:59

KurtYoung reviewed Apr 13, 2021

View reviewed changes

hameizi force-pushed the master branch from a55cd02 to 2535701 Compare April 13, 2021 13:03

KurtYoung reviewed Apr 13, 2021

View reviewed changes

[FLINK-22207][connectors/hive]Hive Catalog retrieve Flink Properties …

8eaeab3

…error

hameizi force-pushed the master branch from 2535701 to 8eaeab3 Compare April 14, 2021 05:47

KurtYoung approved these changes Apr 14, 2021

View reviewed changes

KurtYoung merged commit c29ee91 into apache:master Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-22207][connectors/hive]Hive Catalog retrieve Flink Properties … #15564

[FLINK-22207][connectors/hive]Hive Catalog retrieve Flink Properties … #15564

hameizi commented Apr 12, 2021

flinkbot commented Apr 12, 2021 •

edited

flinkbot commented Apr 12, 2021 •

edited

KurtYoung left a comment

hameizi commented Apr 12, 2021 •

edited

KurtYoung commented Apr 13, 2021

hameizi commented Apr 13, 2021

KurtYoung Apr 13, 2021

hameizi Apr 13, 2021 •

edited

KurtYoung left a comment

KurtYoung Apr 13, 2021

KurtYoung Apr 13, 2021

hameizi Apr 13, 2021 •

edited

hameizi Apr 13, 2021

KurtYoung Apr 13, 2021

hameizi commented Apr 13, 2021

KurtYoung Apr 13, 2021

KurtYoung commented Apr 14, 2021

hameizi commented Apr 14, 2021

KurtYoung left a comment

KurtYoung commented Apr 14, 2021

hameizi commented Apr 14, 2021

[FLINK-22207][connectors/hive]Hive Catalog retrieve Flink Properties … #15564

[FLINK-22207][connectors/hive]Hive Catalog retrieve Flink Properties … #15564

Conversation

hameizi commented Apr 12, 2021

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Apr 12, 2021 • edited

Automated Checks

Review Progress

flinkbot commented Apr 12, 2021 • edited

CI report:

KurtYoung left a comment

Choose a reason for hiding this comment

hameizi commented Apr 12, 2021 • edited

KurtYoung commented Apr 13, 2021

hameizi commented Apr 13, 2021

KurtYoung Apr 13, 2021

Choose a reason for hiding this comment

hameizi Apr 13, 2021 • edited

Choose a reason for hiding this comment

KurtYoung left a comment

Choose a reason for hiding this comment

KurtYoung Apr 13, 2021

Choose a reason for hiding this comment

KurtYoung Apr 13, 2021

Choose a reason for hiding this comment

hameizi Apr 13, 2021 • edited

Choose a reason for hiding this comment

hameizi Apr 13, 2021

Choose a reason for hiding this comment

KurtYoung Apr 13, 2021

Choose a reason for hiding this comment

hameizi commented Apr 13, 2021

KurtYoung Apr 13, 2021

Choose a reason for hiding this comment

KurtYoung commented Apr 14, 2021

hameizi commented Apr 14, 2021

KurtYoung left a comment

Choose a reason for hiding this comment

KurtYoung commented Apr 14, 2021

hameizi commented Apr 14, 2021

flinkbot commented Apr 12, 2021 •

edited

flinkbot commented Apr 12, 2021 •

edited

hameizi commented Apr 12, 2021 •

edited

hameizi Apr 13, 2021 •

edited

hameizi Apr 13, 2021 •

edited