-
Notifications
You must be signed in to change notification settings - Fork 458
pass catalog sensitive info when create fluss catalog #1937
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
luoyuxia
left a comment
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.
@xx789633 Thanks for the pr. But it should allow pass all options instead of sensitive table options only. I image some thing like:
create catalog t1 with (
'paimon.k1' = 'v1',
'paimon.k2 = 'v2'
)
to enable pass 'k1' = 'v1' as well as 'k2 = 'v2
|
You are correct @luoyuxia . Let me rename it. |
| helper.validateExcept( | ||
| Stream.concat( | ||
| Stream.of(CLIENT_SECURITY_PREFIX), | ||
| Arrays.stream(DataLakeFormat.values()) | ||
| .map(DataLakeFormat::toString)) | ||
| .toArray(String[]::new)); |
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.
I suggest to make it extensiable. Thus if the subclass want to support new configs, they don't need to overwrite createCatalog.
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());
}
}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.
Good suggestion. Done.
luoyuxia
left a comment
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.
@xx789633 Thanks for the pr. Left minor comments. PTAL
Also, is it possible to document this behavior that users can pass lake related properties to Flink Catalog via SQL DDL?
| Collections.singletonList(ConfigOptions.TABLE_DATALAKE_ENABLED.key()); | ||
| } | ||
|
|
||
| public static final Set<String> SENSITIVE_TABLE_OPTIONS = new HashSet<>(); |
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.
why move to here?
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.
it seems similar configs like ALTERABLE_TABLE_OPTIONS are all in this file.
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.
But currently, I don't want to expose them in FlussConfigUtils...
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.
fixed
| .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); | ||
| } | ||
|
|
||
| public static <V> Map<String, V> extractPrefix( |
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.
why introducr a new method? Can't we reuse extractPrefix(Map<String, V> originalMap, String prefix)?
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.
I suggest to handle in FlinkCatalogFactory. The method is hard to reuse...
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.
done. just reuse extractPrefix(Map<String, V> originalMap, String prefix).
| + "' is set."); | ||
| } | ||
| String dataLakePrefix = lakeFormat.toString() + "."; | ||
| Map<String, String> catalogProperties = |
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.
Can be
Map<String, String> catalogProperties =
PropertiesUtils.extractAndRemovePrefix(
lakeCatalogProperties, lakeFormat + ".");
?
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.
done
| securityMap.put("client.security.sasl.username", "root"); | ||
| securityMap.put("client.security.sasl.password", "password"); | ||
|
|
||
| Map<String, String> lakeCatalogMap = new HashMap<>(); |
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.
add test in FlinkCatalogITCase to verify SQL can pass the properties.
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.
a new test testCreateCatalogWithLakeProperties is added.
| protected final String bootstrapServers; | ||
| protected final Map<String, String> securityConfigs; | ||
| protected final LakeFlinkCatalog lakeFlinkCatalog; | ||
| protected final Map<String, String> lakeCatalogProperties; |
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.
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.
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.
make sense. done.
|
doc is updated. please take a look @luoyuxia |
luoyuxia
left a comment
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.
@xx789633 Left some comments again
| Map<String, String> securityConfigs) { | ||
| super(name, defaultDatabase, bootstrapServers, classLoader, securityConfigs); | ||
| Map<String, String> securityConfigs, | ||
| Supplier<Map<String, String>> lakeCatalogProperties) { |
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.
| Supplier<Map<String, String>> lakeCatalogProperties) { | |
| Supplier<Map<String, String>> lakeCatalogPropertiesSupplier) { |
| String bootstrapServers, | ||
| ClassLoader classLoader, | ||
| Map<String, String> securityConfigs, | ||
| Supplier<Map<String, String>> lakeCatalogProperties, |
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.
dito
| protected final String bootstrapServers; | ||
| protected final Map<String, String> securityConfigs; | ||
| protected final LakeFlinkCatalog lakeFlinkCatalog; | ||
| protected final Supplier<Map<String, String>> lakeCatalogProperties; |
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.
dito
| objectPath.getDatabaseName(), | ||
| tableName, | ||
| tableInfo.getProperties(), | ||
| lakeCatalogProperties); |
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.
nit: we can get in here directly
lakeCatalogProperties.get()
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.
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.
| } | ||
|
|
||
| @VisibleForTesting | ||
| public Supplier<Map<String, String>> getLakeCatalogProperties() { |
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.
| public Supplier<Map<String, String>> getLakeCatalogProperties() { | |
| public Map<String, String> getLakeCatalogProperties() { |
…nk catalog (apache#1937)" This reverts commit 35d92c5.
…nk catalog (apache#1937)" This reverts commit 35d92c5.
Purpose
Linked issue: close #1903
Brief change log
Tests
API and Format
Documentation