Copilot commented on code in PR #2924:
URL: https://github.com/apache/fluss/pull/2924#discussion_r2986376102
##########
website/docs/maintenance/operations/updating-configs.md:
##########
@@ -27,13 +28,14 @@ Here is a code snippet to demonstrate how to update the
cluster configurations u
```java
// Enable lakehouse storage with Paimon format
admin.alterClusterConfigs(
- Collections.singletonList(
+ Arrays.asList(
+ new AlterConfig(DATALAKE_ENABLED.key(), "true",
AlterConfigOpType.SET),
new AlterConfig(DATALAKE_FORMAT.key(), "paimon",
AlterConfigOpType.SET)));
// Disable lakehouse storage
admin.alterClusterConfigs(
Collections.singletonList(
- new AlterConfig(DATALAKE_FORMAT.key(), "paimon",
AlterConfigOpType.DELETE)));
+ new AlterConfig(DATALAKE_ENABLED.key(), "false",
AlterConfigOpType.SET)));
Review Comment:
The “Disable lakehouse storage” example sets only `datalake.enabled=false`,
but earlier in this page you state that when `datalake.enabled` is explicitly
configured, `datalake.format` must also be configured. As a standalone snippet
this is inconsistent and may fail if the cluster didn’t already have
`datalake.format` set. Consider either adding a note that this assumes
`datalake.format` is already configured, or include setting/keeping
`datalake.format` in the disable example.
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/LakeCatalogDynamicLoader.java:
##########
@@ -63,17 +66,37 @@ public void validate(Configuration newConfig) throws
ConfigException {
newConfig.getOptional(DATALAKE_FORMAT).isPresent()
? newConfig.get(DATALAKE_FORMAT)
: currentConfiguration.get(DATALAKE_FORMAT);
+
+ // TODO: validate(...) only sees the merged effective cluster config,
so it cannot
+ // detect the case where a user enables datalake.enabled and unsets
+ // datalake.format in the same dynamic config change. This may leave
the cluster
+ // with datalake.enabled set but no datalake.format. Fixing this
likely requires
+ // extending the validate/reconfigure framework to expose the
incremental change
+ // set, rather than only the merged result. We accept this for now
because
+ // table-level enablement is still validated, and enabling datalake
for a table
+ // will fail if datalake.format is not configured.
+ Optional<Boolean> optDataLakeEnabled =
newConfig.getOptional(DATALAKE_ENABLED);
+ if (optDataLakeEnabled.isPresent()
+ && optDataLakeEnabled.get()
+ && newDatalakeFormat == null) {
+ throw new ConfigException(
+ String.format(
+ "'%s' must be configured when '%s' is explicitly
set.",
+ DATALAKE_FORMAT.key(), DATALAKE_ENABLED.key()));
+ }
Review Comment:
The `datalake.enabled`/`datalake.format` validation currently only triggers
when `datalake.enabled` is explicitly set to `true`
(`optDataLakeEnabled.get()`), but the new semantics/docs/tests require
`datalake.format` whenever `datalake.enabled` is explicitly configured (true or
false). This will make
`DynamicConfigChangeTest#testExplicitDataLakeEnabledRequiresDataLakeFormat`
fail. Suggest removing the `optDataLakeEnabled.get()` condition and checking
format presence whenever `optDataLakeEnabled.isPresent()`.
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/LakeCatalogDynamicLoader.java:
##########
@@ -63,17 +66,37 @@ public void validate(Configuration newConfig) throws
ConfigException {
newConfig.getOptional(DATALAKE_FORMAT).isPresent()
? newConfig.get(DATALAKE_FORMAT)
: currentConfiguration.get(DATALAKE_FORMAT);
Review Comment:
In validate(), `newDatalakeFormat` is derived by falling back to
`currentConfiguration.get(DATALAKE_FORMAT)` when the key is absent in
`newConfig`. But `newConfig` is already the merged effective config (see
DynamicServerConfig), so this fallback prevents detecting a format removal and
can cause prefix validation / the datalake.enabled requirement check to behave
incorrectly when `datalake.format` is unset. Consider using only
`newConfig.getOptional(DATALAKE_FORMAT).orElse(null)` here (and base
validations on presence in `newConfig`).
```suggestion
newConfig.getOptional(DATALAKE_FORMAT).orElse(null);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
