[#6302] refactor:Optimize Flink connector properties converter #6303
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
from
aecdd3a to
03c0d52
Compare
|
fixed ci error : #6305 |
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
from
03c0d52 to
97bc2a7
Compare
|
@FANNG1 PTAL ， thanks |
FANNG1
reviewed
Jan 20, 2025
...ink/src/main/java/org/apache/gravitino/flink/connector/paimon/PaimonPropertiesConverter.java
Outdated
Show resolved
Hide resolved
FANNG1
reviewed
Jan 20, 2025
...-connector/flink/src/main/java/org/apache/gravitino/flink/connector/PropertiesConverter.java
Outdated
Show resolved
Hide resolved
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
3 times, most recently
from
f576a21 to
92d0edb
Compare
|
@FANNG1 Fixed all ， PTAL again, thanks |
FANNG1
reviewed
Jan 21, 2025
...-connector/flink/src/main/java/org/apache/gravitino/flink/connector/PropertiesConverter.java
Outdated
Show resolved
Hide resolved
FANNG1
reviewed
Jan 21, 2025
...-connector/flink/src/main/java/org/apache/gravitino/flink/connector/PropertiesConverter.java
Show resolved
Hide resolved
FANNG1
reviewed
Jan 21, 2025
...c/test/java/org/apache/gravitino/flink/connector/iceberg/TestIcebergPropertiesConverter.java
Show resolved
Hide resolved
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
2 times, most recently
from
dbeb10b to
364591c
Compare
FANNG1
reviewed
Jan 22, 2025
...c/test/java/org/apache/gravitino/flink/connector/iceberg/TestIcebergPropertiesConverter.java
Show resolved
Hide resolved
FANNG1
reviewed
Jan 22, 2025
...c/test/java/org/apache/gravitino/flink/connector/iceberg/TestIcebergPropertiesConverter.java
Show resolved
Hide resolved
|
LGTM except minor comments |
FANNG1
reviewed
Jan 23, 2025
...-connector/flink/src/main/java/org/apache/gravitino/flink/connector/PropertiesConverter.java
Show resolved
Hide resolved
FANNG1
reviewed
Jan 23, 2025
| }); | ||
| return flinkCatalogProperties; | ||
| public String transformPropertyToFlinkCatalog(String configKey) { | ||
| return GRAVITINO_CONFIG_TO_HIVE.getOrDefault(configKey, configKey); |
There was a problem hiding this comment.
How about using GRAVITINO_CONFIG_TO_HIVE.get(configKey)?
There was a problem hiding this comment.
transformPropertyToFlinkCatalog return null if the config key not in the map, WDYT?
FANNG1
reviewed
Jan 23, 2025
...k/src/main/java/org/apache/gravitino/flink/connector/iceberg/IcebergPropertiesConverter.java
Show resolved
Hide resolved
FANNG1
reviewed
Jan 23, 2025
| @@ -35,7 +35,7 @@ public void testToGravitinoCatalogProperties() { | |||
| Configuration configuration = | |||
| Configuration.fromMap( | |||
| ImmutableMap.of( | |||
| "hive-conf-dir", | |||
There was a problem hiding this comment.
better to pass hive-conf-dir not flink.bypass.hive-conf-dir when creating a hive catalog from Flink SQL
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
from
3747563 to
fc250a6
Compare
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
from
fc250a6 to
6fb45d6
Compare
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
4 times, most recently
from
266fda7 to
8c82109
Compare
sunxiaojian
force-pushed
the
extract-flink-converter-logic
branch
from
8c82109 to
e01414b
Compare
|
LGTM |
FANNG1
approved these changes
Jan 23, 2025
|
@sunxiaojian thanks for your work to make code cleaner. |
Abyss-lord
pushed a commit
to Abyss-lord/gravitino
that referenced
this pull request
Feb 6, 2025
…pache#6303) ### What changes were proposed in this pull request? - Unified params format, add flink.bypass. prefix, especially Iceberg. - Extract the flink.bypass. logic to the PropertiesConverter, and the extension logic does not care about this operation ### Why are the changes needed? Fix: [#(6302)](apache#6302) ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? N/A
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Why are the changes needed?
Fix: #(6302)
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
N/A