You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fluss.apache.org by "xx789633 (via GitHub)" <gi...@apache.org> on 2025/11/05 10:06:42 UTC
[PR] pass catalog sensitive info when create fluss catalog [fluss]
xx789633 opened a new pull request, #1937:
URL: https://github.com/apache/fluss/pull/1937
<!--
*Thank you very much for contributing to Fluss - we are happy that you want to help us improve Fluss. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [GitHub issue](https://github.com/apache/fluss/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no issue.
- Name the pull request in the format "[component] Title of the pull request", where *[component]* should be replaced by the name of the component being changed. Typically, this corresponds to the component label assigned to the issue (e.g., [kv], [log], [client], [flink]). Skip *[component]* if you are unsure about which is the best component.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes.
- Each pull request should address only one issue, not mix up code from multiple issues.
**(The sections below can be removed for hotfixes or typos)**
-->
### Purpose
<!-- Linking this pull request to the issue -->
Linked issue: close #1903
<!-- What is the purpose of the change -->
### Brief change log
<!-- Please describe the changes made in this pull request and explain how they address the issue -->
### Tests
<!-- List UT and IT cases to verify this change -->
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on PR #1937:
URL: https://github.com/apache/fluss/pull/1937#issuecomment-3659711303
doc is updated. please take a look @luoyuxia
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on PR #1937:
URL: https://github.com/apache/fluss/pull/1937#issuecomment-3512391791
You are correct @luoyuxia . Let me rename it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "luoyuxia (via GitHub)" <gi...@apache.org>.
luoyuxia commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2622363708
##########
fluss-common/src/main/java/org/apache/fluss/config/FlussConfigUtils.java:
##########
@@ -45,6 +47,14 @@ public class FlussConfigUtils {
Collections.singletonList(ConfigOptions.TABLE_DATALAKE_ENABLED.key());
}
+ public static final Set<String> SENSITIVE_TABLE_OPTIONS = new HashSet<>();
Review Comment:
But currently, I don't want to expose them in `FlussConfigUtils`...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2622355699
##########
fluss-common/src/main/java/org/apache/fluss/config/FlussConfigUtils.java:
##########
@@ -45,6 +47,14 @@ public class FlussConfigUtils {
Collections.singletonList(ConfigOptions.TABLE_DATALAKE_ENABLED.key());
}
+ public static final Set<String> SENSITIVE_TABLE_OPTIONS = new HashSet<>();
Review Comment:
it seems similar configs like `ALTERABLE_TABLE_OPTIONS` are all in this file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "luoyuxia (via GitHub)" <gi...@apache.org>.
luoyuxia commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2623272292
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalog.java:
##########
@@ -772,4 +785,9 @@ public Procedure getProcedure(ObjectPath procedurePath)
public Map<String, String> getSecurityConfigs() {
return securityConfigs;
}
+
+ @VisibleForTesting
+ public Supplier<Map<String, String>> getLakeCatalogProperties() {
Review Comment:
```suggestion
public Map<String, String> getLakeCatalogProperties() {
```
##########
fluss-flink/fluss-flink-2.1/src/main/java/org/apache/fluss/flink/catalog/Flink21Catalog.java:
##########
@@ -53,13 +61,15 @@ public Flink21Catalog(
String bootstrapServers,
ClassLoader classLoader,
Map<String, String> securityConfigs,
+ Supplier<Map<String, String>> lakeCatalogProperties,
Review Comment:
dito
##########
fluss-flink/fluss-flink-2.1/src/main/java/org/apache/fluss/flink/catalog/Flink21Catalog.java:
##########
@@ -42,8 +43,15 @@ public Flink21Catalog(
String defaultDatabase,
String bootstrapServers,
ClassLoader classLoader,
- Map<String, String> securityConfigs) {
- super(name, defaultDatabase, bootstrapServers, classLoader, securityConfigs);
+ Map<String, String> securityConfigs,
+ Supplier<Map<String, String>> lakeCatalogProperties) {
Review Comment:
```suggestion
Supplier<Map<String, String>> lakeCatalogPropertiesSupplier) {
```
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalog.java:
##########
@@ -115,6 +116,7 @@ public class FlinkCatalog extends AbstractCatalog {
protected final String bootstrapServers;
protected final Map<String, String> securityConfigs;
protected final LakeFlinkCatalog lakeFlinkCatalog;
+ protected final Supplier<Map<String, String>> lakeCatalogProperties;
Review Comment:
dito
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalog.java:
##########
@@ -312,8 +318,12 @@ public CatalogBaseTable getTable(ObjectPath objectPath)
objectPath.getDatabaseName(),
tableName.split("\\" + LAKE_TABLE_SPLITTER)[0])));
}
+
return getLakeTable(
- objectPath.getDatabaseName(), tableName, tableInfo.getProperties());
+ objectPath.getDatabaseName(),
+ tableName,
+ tableInfo.getProperties(),
+ lakeCatalogProperties);
Review Comment:
nit: we can get in here directly
lakeCatalogProperties.get()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2622348190
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalog.java:
##########
@@ -115,6 +115,7 @@ public class FlinkCatalog extends AbstractCatalog {
protected final String bootstrapServers;
protected final Map<String, String> securityConfigs;
protected final LakeFlinkCatalog lakeFlinkCatalog;
+ protected final Map<String, String> lakeCatalogProperties;
Review Comment:
make sense. done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "luoyuxia (via GitHub)" <gi...@apache.org>.
luoyuxia merged PR #1937:
URL: https://github.com/apache/fluss/pull/1937
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "luoyuxia (via GitHub)" <gi...@apache.org>.
luoyuxia commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2618714151
##########
fluss-common/src/main/java/org/apache/fluss/utils/PropertiesUtils.java:
##########
@@ -57,6 +59,21 @@ public static <V> Map<String, V> extractPrefix(Map<String, V> originalMap, Strin
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}
+ public static <V> Map<String, V> extractPrefix(
Review Comment:
why introducr a new method? Can't we reuse `extractPrefix(Map<String, V> originalMap, String prefix)`?
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/lake/LakeFlinkCatalog.java:
##########
@@ -69,12 +71,28 @@ public Catalog getLakeCatalog(Configuration tableOptions) {
+ ConfigOptions.TABLE_DATALAKE_FORMAT.key()
+ "' is set.");
}
+ String dataLakePrefix = lakeFormat.toString() + ".";
+ Map<String, String> catalogProperties =
Review Comment:
Can be
```
Map<String, String> catalogProperties =
PropertiesUtils.extractAndRemovePrefix(
lakeCatalogProperties, lakeFormat + ".");
```
?
##########
fluss-common/src/main/java/org/apache/fluss/utils/PropertiesUtils.java:
##########
@@ -57,6 +59,21 @@ public static <V> Map<String, V> extractPrefix(Map<String, V> originalMap, Strin
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}
+ public static <V> Map<String, V> extractPrefix(
Review Comment:
I suggest to handle in `FlinkCatalogFactory`. The method is hard to reuse...
##########
fluss-common/src/main/java/org/apache/fluss/config/FlussConfigUtils.java:
##########
@@ -45,6 +47,14 @@ public class FlussConfigUtils {
Collections.singletonList(ConfigOptions.TABLE_DATALAKE_ENABLED.key());
}
+ public static final Set<String> SENSITIVE_TABLE_OPTIONS = new HashSet<>();
Review Comment:
why move to here?
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalog.java:
##########
@@ -115,6 +115,7 @@ public class FlinkCatalog extends AbstractCatalog {
protected final String bootstrapServers;
protected final Map<String, String> securityConfigs;
protected final LakeFlinkCatalog lakeFlinkCatalog;
+ protected final Map<String, String> lakeCatalogProperties;
Review Comment:
Could you please make `lakeCatalogProperties` to be `Supplier<Map<String, String>> lakeCatalogPropertiesProvider`. The reason it that it make it more plugable. In our inner use, we will retrieve the lake catalog related from flink conf, but we only want to do the retrieve if it's required.
##########
fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/catalog/FlinkCatalogFactoryTest.java:
##########
@@ -75,7 +76,12 @@ public void testCreateCatalog() {
securityMap.put("client.security.sasl.username", "root");
securityMap.put("client.security.sasl.password", "password");
+ Map<String, String> lakeCatalogMap = new HashMap<>();
Review Comment:
add test in `FlinkCatalogITCase` to verify SQL can pass the properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2623283685
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalog.java:
##########
@@ -312,8 +318,12 @@ public CatalogBaseTable getTable(ObjectPath objectPath)
objectPath.getDatabaseName(),
tableName.split("\\" + LAKE_TABLE_SPLITTER)[0])));
}
+
return getLakeTable(
- objectPath.getDatabaseName(), tableName, tableInfo.getProperties());
+ objectPath.getDatabaseName(),
+ tableName,
+ tableInfo.getProperties(),
+ lakeCatalogProperties);
Review Comment:
Acquiring the properties here will frequently invoke the supplier function and bring some overhead. Actually, the properties are only needed when creating the lake catalog.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2622350311
##########
fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/catalog/FlinkCatalogFactoryTest.java:
##########
@@ -75,7 +76,12 @@ public void testCreateCatalog() {
securityMap.put("client.security.sasl.username", "root");
securityMap.put("client.security.sasl.password", "password");
+ Map<String, String> lakeCatalogMap = new HashMap<>();
Review Comment:
a new test `testCreateCatalogWithLakeProperties` is added.
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/lake/LakeFlinkCatalog.java:
##########
@@ -69,12 +71,28 @@ public Catalog getLakeCatalog(Configuration tableOptions) {
+ ConfigOptions.TABLE_DATALAKE_FORMAT.key()
+ "' is set.");
}
+ String dataLakePrefix = lakeFormat.toString() + ".";
+ Map<String, String> catalogProperties =
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on PR #1937:
URL: https://github.com/apache/fluss/pull/1937#issuecomment-3515320305
For those lake catalog properties, should they start with a unified prefix, like "table.datalake." or "datalake"? @luoyuxia
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "LiebingYu (via GitHub)" <gi...@apache.org>.
LiebingYu commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2523077190
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalogFactory.java:
##########
@@ -54,15 +57,22 @@ public Set<ConfigOption<?>> optionalOptions() {
public FlinkCatalog createCatalog(Context context) {
final FactoryUtil.CatalogFactoryHelper helper =
FactoryUtil.createCatalogFactoryHelper(this, context);
- helper.validateExcept(CLIENT_SECURITY_PREFIX);
+ helper.validateExcept(
+ Stream.concat(
+ Stream.of(CLIENT_SECURITY_PREFIX),
+ Arrays.stream(DataLakeFormat.values())
+ .map(DataLakeFormat::toString))
+ .toArray(String[]::new));
Review Comment:
I suggest to make it extensiable. Thus if the subclass want to support new configs, they don't need to overwrite `createCatalog`.
```java
protected static final List<String> ALLOW_CATALOG_CONFIG_PREFIX = new ArrayList<>();
static {
ALLOW_CATALOG_CONFIG_PREFIX.add(CLIENT_SECURITY_PREFIX);
// support lake catalog options
for (DataLakeFormat value : DataLakeFormat.values()) {
ALLOW_CATALOG_CONFIG_PREFIX.add(value.toString());
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2525537495
##########
fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalogFactory.java:
##########
@@ -54,15 +57,22 @@ public Set<ConfigOption<?>> optionalOptions() {
public FlinkCatalog createCatalog(Context context) {
final FactoryUtil.CatalogFactoryHelper helper =
FactoryUtil.createCatalogFactoryHelper(this, context);
- helper.validateExcept(CLIENT_SECURITY_PREFIX);
+ helper.validateExcept(
+ Stream.concat(
+ Stream.of(CLIENT_SECURITY_PREFIX),
+ Arrays.stream(DataLakeFormat.values())
+ .map(DataLakeFormat::toString))
+ .toArray(String[]::new));
Review Comment:
Good suggestion. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2622389241
##########
fluss-common/src/main/java/org/apache/fluss/config/FlussConfigUtils.java:
##########
@@ -45,6 +47,14 @@ public class FlussConfigUtils {
Collections.singletonList(ConfigOptions.TABLE_DATALAKE_ENABLED.key());
}
+ public static final Set<String> SENSITIVE_TABLE_OPTIONS = new HashSet<>();
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] pass catalog sensitive info when create fluss catalog [fluss]
Posted by "xx789633 (via GitHub)" <gi...@apache.org>.
xx789633 commented on code in PR #1937:
URL: https://github.com/apache/fluss/pull/1937#discussion_r2622351969
##########
fluss-common/src/main/java/org/apache/fluss/utils/PropertiesUtils.java:
##########
@@ -57,6 +59,21 @@ public static <V> Map<String, V> extractPrefix(Map<String, V> originalMap, Strin
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
}
+ public static <V> Map<String, V> extractPrefix(
Review Comment:
done. just reuse `extractPrefix(Map<String, V> originalMap, String prefix)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org