Copilot commented on code in PR #2924:
URL: https://github.com/apache/fluss/pull/2924#discussion_r2986160383
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/LakeCatalogDynamicLoader.java:
##########
@@ -63,17 +66,35 @@ 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
Review Comment:
LakeCatalogDynamicLoader.validate(): `newDatalakeFormat` falls back to
`currentConfiguration.get(DATALAKE_FORMAT)` when the new effective config has
no `datalake.format`. That prevents the intended “format is null => skip prefix
validation” behavior and can block disabling `datalake.format` if any
`datalake.*` keys from a different format are present. Since `newConfig` is
already the merged effective config, derive `newDatalakeFormat` only from
`newConfig` (no fallback to current).
```suggestion
newConfig.getOptional(DATALAKE_FORMAT).orElse(null);
// TODO: validate(...) only sees the merged effective cluster
config, so it cannot
// detect the case where a user enables datalake.enabled and unsets
// detect the case where a user enables datalake.enabled and unsets
```
##########
fluss-server/src/main/java/org/apache/fluss/server/utils/TableDescriptorValidation.java:
##########
@@ -118,6 +124,23 @@ public static void validateTableDescriptor(TableDescriptor
tableDescriptor, int
checkTieredLog(tableConf);
checkPartition(tableConf, tableDescriptor.getPartitionKeys(),
schema.getRowType());
checkSystemColumns(schema.getRowType());
+ checkTableLakeFormatMatchesCluster(tableConf, clusterDataLakeFormat);
+ }
+
+ private static void checkTableLakeFormatMatchesCluster(
+ Configuration tableConf, @Nullable DataLakeFormat
clusterDataLakeFormat) {
+ Optional<DataLakeFormat> tableDataLakeFormat =
+ tableConf.getOptional(ConfigOptions.TABLE_DATALAKE_FORMAT);
+ if (tableDataLakeFormat.isPresent() && tableDataLakeFormat.get() !=
clusterDataLakeFormat) {
+ throw new InvalidConfigException(
+ String.format(
+ "'%s' ('%s') must match cluster '%s' ('%s') when
'%s' is enabled.",
+ ConfigOptions.TABLE_DATALAKE_FORMAT.key(),
+ tableDataLakeFormat.get(),
+ ConfigOptions.DATALAKE_FORMAT.key(),
+ clusterDataLakeFormat,
+ ConfigOptions.TABLE_DATALAKE_ENABLED.key()));
Review Comment:
checkTableLakeFormatMatchesCluster(): this rejects creating/altering a table
that sets `table.datalake.format` when the cluster has no `datalake.format`
configured (`clusterDataLakeFormat == null`). That breaks the
documented/IT-case flow of pre-setting a table’s lake format before the cluster
enables lakehouse. Only enforce the match when the cluster has a non-null
`datalake.format` (and adjust the error message accordingly).
```suggestion
if (clusterDataLakeFormat != null
&& tableDataLakeFormat.isPresent()
&& tableDataLakeFormat.get() != clusterDataLakeFormat) {
throw new InvalidConfigException(
String.format(
"'%s' ('%s') must match cluster '%s' ('%s') when
the cluster has a"
+ " datalake format configured.",
ConfigOptions.TABLE_DATALAKE_FORMAT.key(),
tableDataLakeFormat.get(),
ConfigOptions.DATALAKE_FORMAT.key(),
clusterDataLakeFormat));
```
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/LakeCatalogDynamicLoader.java:
##########
@@ -63,17 +66,35 @@ 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.
+ boolean explicitDataLakeEnabled =
newConfig.getOptional(DATALAKE_ENABLED).orElse(false);
+ if (explicitDataLakeEnabled && newDatalakeFormat != null) {
+ throw new ConfigException(
+ String.format(
+ "'%s' must be configured when '%s' is explicitly
set.",
+ DATALAKE_FORMAT.key(), DATALAKE_ENABLED.key()));
+ }
Review Comment:
LakeCatalogDynamicLoader.validate(): the check for requiring datalake.format
when datalake.enabled is explicitly configured is currently inverted and also
only triggers when the value is `true`. As written, it throws when a format
*is* present and misses the intended cases (e.g., `datalake.enabled=false` with
missing format). Update the logic to detect explicit configuration via
`newConfig.getOptional(DATALAKE_ENABLED).isPresent()` (not the boolean value)
and throw only when the effective `datalake.format` is absent.
##########
website/docs/maintenance/operations/upgrade-notes-0.10.md:
##########
@@ -4,3 +4,36 @@ sidebar_position: 4
---
# Upgrade Notes from v0.9 to v0.10
+
+## New `datalake.enabled` Cluster Configuration
+
+Starting from v0.10, Fluss introduces the cluster-level configuration
`datalake.enabled` to control whether the cluster is ready to create and manage
lakehouse tables.
+
+### Behavior Changes
+
+- If `datalake.enabled` is unset, Fluss keeps the legacy behavior: configuring
`datalake.format` alone also enables lakehouse tables.
+- If `datalake.enabled = false`, Fluss only pre-binds the lake format for
newly created tables and does not allow lakehouse tables yet.
+- If `datalake.enabled = true`, Fluss fully enables lakehouse tables.
Review Comment:
PR description says “Documentation: no”, but this PR adds/updates multiple
docs pages (quickstart, tiered-storage, upgrade notes, updating-configs,
observability quickstart). Please update the PR description (or the
“Documentation” section) to reflect that documentation was changed so
reviewers/users don’t miss it.
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/LakeCatalogDynamicLoader.java:
##########
@@ -136,19 +175,31 @@ public LakeCatalogContainer(
Configuration configuration,
@Nullable PluginManager pluginManager,
boolean isCoordinator) {
+ this.clusterDataLakeTableEnabled =
isClusterDataLakeTableEnabled(configuration);
this.dataLakeFormat =
configuration.getOptional(DATALAKE_FORMAT).orElse(null);
this.lakeCatalog =
isCoordinator ? createLakeCatalog(configuration,
pluginManager) : null;
this.defaultTableLakeOptions =
LakeStorageUtils.generateDefaultTableLakeOptions(configuration);
- if (isCoordinator && ((dataLakeFormat == null) != (lakeCatalog ==
null))) {
+ if (isCoordinator && clusterDataLakeTableEnabled == (lakeCatalog
== null)) {
throw new ConfigException(
String.format(
- "dataLakeFormat and lakeCatalog must both be
null or both non-null, but dataLakeFormat is %s, lakeCatalog is %s.",
- dataLakeFormat, lakeCatalog));
+ "clusterDataLakeTableEnabled and lakeCatalog
must both be false/null or true/non-null, but clusterDataLakeTableEnabled is
%s, lakeCatalog is %s.",
+ clusterDataLakeTableEnabled, lakeCatalog));
}
}
+ static boolean isClusterDataLakeTableEnabled(Configuration
configuration) {
+ Optional<Boolean> explicitDataLakeEnabled =
configuration.getOptional(DATALAKE_ENABLED);
+ // if datalake.enabled no set, use datalake.format for legacy
cluster behavior
Review Comment:
Minor grammar: comment says “if datalake.enabled no set”; should be “not
set”.
```suggestion
// if datalake.enabled not set, use datalake.format for legacy
cluster behavior
```
--
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]
