Copilot commented on code in PR #2924:
URL: https://github.com/apache/fluss/pull/2924#discussion_r2986529382
##########
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 to true.",
+ DATALAKE_FORMAT.key(), DATALAKE_ENABLED.key()));
+ }
+
// If datalake format is not set, skip prefix validation so that users
can disable or enable
// datalake format without re-supplying all datalake-prefixed
properties.
Review Comment:
The validation currently enforces `datalake.format` only when
`datalake.enabled` is explicitly set to `true`, but the new semantics/docs say
`datalake.format` must be configured whenever `datalake.enabled` is explicitly
set (true *or* false). Consider validating
`newConfig.getOptional(DATALAKE_ENABLED).isPresent()` implies
`newConfig.getOptional(DATALAKE_FORMAT).isPresent()`, and adjust the exception
message accordingly.
```suggestion
Optional<DataLakeFormat> optDataLakeFormat =
newConfig.getOptional(DATALAKE_FORMAT);
if (optDataLakeEnabled.isPresent() &&
!optDataLakeFormat.isPresent()) {
throw new ConfigException(
String.format(
"'%s' must be configured whenever '%s' is
explicitly set.",
DATALAKE_FORMAT.key(), DATALAKE_ENABLED.key()));
}
// If datalake format is not set, skip prefix validation so that
users can disable or enable
// datalake format without re-supplying all datalake-prefixed
properties.
// datalake format without re-supplying all datalake-prefixed
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
