Copilot commented on code in PR #1172:
URL: https://github.com/apache/fluss/pull/1172#discussion_r2862144126
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/MetadataManager.java:
##########
@@ -136,6 +137,77 @@ public void createDatabase(
}
}
+ public void alterDatabaseProperties(
+ String databaseName,
+ DatabasePropertyChanges databasePropertyChanges,
+ boolean ignoreIfNotExists) {
+ try {
+ // Check if database exists
+ if (!databaseExists(databaseName)) {
+ if (ignoreIfNotExists) {
+ return;
+ }
+ throw new DatabaseNotExistException("Database " + databaseName
+ " not exists.");
+ }
+
+ DatabaseInfo databaseInfo = getDatabase(databaseName);
+ DatabaseDescriptor currentDescriptor =
databaseInfo.getDatabaseDescriptor();
+
+ // Create updated descriptor
+ DatabaseDescriptor newDescriptor =
+ getUpdatedDatabaseDescriptor(currentDescriptor,
databasePropertyChanges);
+
+ if (newDescriptor != null) {
+ // Update the database in ZooKeeper
+ DatabaseRegistration updatedRegistration =
DatabaseRegistration.of(newDescriptor);
+ zookeeperClient.updateDatabase(databaseName,
updatedRegistration);
+ LOG.info("Successfully altered database properties for
database: {}", databaseName);
+ } else {
+ LOG.info(
+ "No properties changed when alter database {}, skip
update.", databaseName);
+ }
+ } catch (Exception e) {
+ if (e instanceof DatabaseNotExistException) {
+ if (ignoreIfNotExists) {
+ return;
+ }
+ throw (DatabaseNotExistException) e;
+ } else if (e instanceof RuntimeException) {
+ throw (RuntimeException) e;
+ } else {
+ throw new FlussRuntimeException("Failed to alter database: " +
databaseName, e);
+ }
+ }
+ }
+
+ private DatabaseDescriptor getUpdatedDatabaseDescriptor(
+ DatabaseDescriptor currentDescriptor, DatabasePropertyChanges
changes) {
+ Map<String, String> newCustomProperties =
+ new HashMap<>(currentDescriptor.getCustomProperties());
+ // set properties
+ newCustomProperties.putAll(changes.customPropertiesToSet);
+ // reset properties
+
newCustomProperties.keySet().removeAll(changes.customPropertiesToReset);
+
+ boolean hasCommentChange =
+
!currentDescriptor.getComment().equals(Optional.ofNullable(changes.commentToSet));
Review Comment:
`hasCommentChange` is derived from `changes.commentToSet`, but
`commentToSet` defaults to null. That means an ALTER that only changes custom
properties (or even an empty change list) will be interpreted as a comment
change and will clear any existing comment. The change model needs a tri-state
(no-op vs set comment vs reset comment), e.g., a `boolean commentTouched` flag
in `DatabasePropertyChanges` that's only set when a comment change is
explicitly requested.
```suggestion
changes.commentToSet != null
&& !currentDescriptor
.getComment()
.equals(Optional.of(changes.commentToSet));
```
##########
fluss-server/src/main/java/org/apache/fluss/server/entity/DatabasePropertyChanges.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.fluss.server.entity;
+
+import javax.annotation.Nullable;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/** To describe the changes of the properties of a database. */
+public class DatabasePropertyChanges {
+
+ public final Map<String, String> customPropertiesToSet;
+ public final Set<String> customPropertiesToReset;
+ public final String commentToSet;
+
+ protected DatabasePropertyChanges(
+ Map<String, String> customPropertiesToSet,
+ Set<String> customPropertiesToReset,
+ @Nullable String commentToSet) {
+ this.customPropertiesToSet = customPropertiesToSet;
+ this.customPropertiesToReset = customPropertiesToReset;
+ this.commentToSet = commentToSet;
+ }
+
+ public static DatabasePropertyChanges.Builder builder() {
+ return new DatabasePropertyChanges.Builder();
+ }
+
+ /** The builder for {@link DatabasePropertyChanges}. */
+ public static class Builder {
+ private final Map<String, String> customPropertiesToSet = new
HashMap<>();
+ private final Set<String> customPropertiesToReset = new HashSet<>();
+
+ private String commentToSet = null;
+
Review Comment:
`commentToSet` defaults to null, but null is also the desired value for a
legitimate "reset comment" operation. As written, `DatabasePropertyChanges`
cannot represent "no comment change" vs "reset comment", which leads to
unintended comment clearing when only properties are altered. Consider adding
an explicit flag (e.g., `commentChanged`/`commentTouched`) or storing the
comment change as an `Optional` wrapper distinct from the value itself.
##########
fluss-server/src/main/java/org/apache/fluss/server/entity/DatabasePropertyChanges.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.fluss.server.entity;
+
+import javax.annotation.Nullable;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/** To describe the changes of the properties of a database. */
+public class DatabasePropertyChanges {
+
+ public final Map<String, String> customPropertiesToSet;
+ public final Set<String> customPropertiesToReset;
+ public final String commentToSet;
+
+ protected DatabasePropertyChanges(
+ Map<String, String> customPropertiesToSet,
+ Set<String> customPropertiesToReset,
+ @Nullable String commentToSet) {
+ this.customPropertiesToSet = customPropertiesToSet;
+ this.customPropertiesToReset = customPropertiesToReset;
+ this.commentToSet = commentToSet;
+ }
+
+ public static DatabasePropertyChanges.Builder builder() {
+ return new DatabasePropertyChanges.Builder();
+ }
+
+ /** The builder for {@link DatabasePropertyChanges}. */
+ public static class Builder {
+ private final Map<String, String> customPropertiesToSet = new
HashMap<>();
+ private final Set<String> customPropertiesToReset = new HashSet<>();
+
+ private String commentToSet = null;
+
+ public void setCustomProperty(String key, String value) {
+ customPropertiesToSet.put(key, value);
+ }
+
+ public void resetCustomProperty(String key) {
+ customPropertiesToReset.add(key);
+ }
+
+ public void setComment(String comment) {
Review Comment:
`Builder#setComment` accepts a non-null `String`, but it is called with
`updateComment.getComment()` which can be null (to reset the comment). Please
update the method signature/annotation to accept `@Nullable` and (ideally) also
record that the comment was explicitly changed so null can mean "reset" rather
than "not provided".
```suggestion
public void setComment(@Nullable String comment) {
```
##########
fluss-common/src/main/java/org/apache/fluss/metadata/DatabaseChange.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.fluss.metadata;
+
+import javax.annotation.Nullable;
+
+import java.util.Objects;
+
+/** {@link DatabaseChange} represents the modification of the Fluss Database.
*/
+public interface DatabaseChange {
+
+ static SetOption set(String key, String value) {
+ return new SetOption(key, value);
+ }
+
+ static ResetOption reset(String key) {
+ return new ResetOption(key);
+ }
+
+ static UpdateComment updateComment(@Nullable String comment) {
+ return new UpdateComment(comment);
+ }
+
+ /**
+ * A database change to set the database option.
+ *
+ * <p>It is equal to the following statement:
+ *
+ * <pre>
+ * ALTER DATABASE <database_name> SET '<key>' =
'<value>';
+ * </pre>
+ */
+ class SetOption implements DatabaseChange {
+
+ private final String key;
+ private final String value;
+
+ private SetOption(String key, String value) {
+ this.key = key;
+ this.value = value;
+ }
+
+ /** Returns the Option key to set. */
+ public String getKey() {
+ return key;
+ }
+
+ /** Returns the Option value to set. */
+ public String getValue() {
+ return value;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof SetOption)) {
+ return false;
+ }
+ SetOption setOption = (SetOption) o;
+ return Objects.equals(key, setOption.key) && Objects.equals(value,
setOption.value);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(key, value);
+ }
+
+ @Override
+ public String toString() {
+ return "SetOption{" + "key='" + key + '\'' + ", value='" + value +
'\'' + '}';
+ }
+ }
+
+ /**
+ * A database change to reset the database option.
+ *
+ * <p>It is equal to the following statement:
+ *
+ * <pre>
+ * ALTER DATABASE <database_name> RESET '<key>'
+ * </pre>
+ */
+ class ResetOption implements DatabaseChange {
+
+ private final String key;
+
+ public ResetOption(String key) {
+ this.key = key;
+ }
+
+ /** Returns the Option key to reset. */
+ public String getKey() {
+ return key;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof ResetOption)) {
+ return false;
+ }
+ ResetOption that = (ResetOption) o;
+ return Objects.equals(key, that.key);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(key);
+ }
+
+ @Override
+ public String toString() {
+ return "ResetOption{" + "key='" + key + '\'' + '}';
+ }
+ }
+
+ /**
+ * A database change to set the database comment.
+ *
+ * <p>It is equal to the following statement:
+ *
+ * <pre>
+ * ALTER DATABASE <database_name> SET COMMENT '<comment>';
+ * </pre>
+ */
+ class UpdateComment implements DatabaseChange {
+
+ private final @Nullable String comment;
+
+ private UpdateComment(@Nullable String comment) {
+ this.comment = comment;
+ }
+
+ /** Returns the comment to set. */
+ @Nullable
+ public String getComment() {
+ return comment;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof UpdateComment)) {
+ return false;
+ }
+ UpdateComment that = (UpdateComment) o;
+ return Objects.equals(comment, that.comment);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(comment);
+ }
+
+ @Override
+ public String toString() {
+ return "SetComment{" + "comment='" + comment + '\'' + '}';
Review Comment:
`UpdateComment#toString()` returns the prefix `SetComment{...}` which
doesn't match the class name / operation (`UpdateComment`). This makes
logs/debug output confusing; consider updating the string to reflect
`UpdateComment` (and include whether it is clearing vs setting).
```suggestion
return "UpdateComment{"
+ (comment == null ? "clearComment" : "comment='" +
comment + '\'')
+ '}';
```
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/MetadataManager.java:
##########
@@ -136,6 +137,77 @@ public void createDatabase(
}
}
+ public void alterDatabaseProperties(
+ String databaseName,
+ DatabasePropertyChanges databasePropertyChanges,
+ boolean ignoreIfNotExists) {
+ try {
+ // Check if database exists
+ if (!databaseExists(databaseName)) {
+ if (ignoreIfNotExists) {
+ return;
+ }
+ throw new DatabaseNotExistException("Database " + databaseName
+ " not exists.");
+ }
+
+ DatabaseInfo databaseInfo = getDatabase(databaseName);
+ DatabaseDescriptor currentDescriptor =
databaseInfo.getDatabaseDescriptor();
+
+ // Create updated descriptor
+ DatabaseDescriptor newDescriptor =
+ getUpdatedDatabaseDescriptor(currentDescriptor,
databasePropertyChanges);
+
+ if (newDescriptor != null) {
+ // Update the database in ZooKeeper
+ DatabaseRegistration updatedRegistration =
DatabaseRegistration.of(newDescriptor);
Review Comment:
`alterDatabaseProperties` builds `DatabaseRegistration` via
`DatabaseRegistration.of(newDescriptor)`, which sets both `createdTime` and
`modifiedTime` to `System.currentTimeMillis()`. This will overwrite the
database's original create time on every ALTER. Consider constructing the
updated registration using the existing `createdTime` from the current DB
registration/info and only bumping `modifiedTime` (similar to how
`TableRegistration#newProperties` preserves `createdTime`).
```suggestion
// Preserve original createdTime and only update modifiedTime
long createdTime = databaseInfo.getCreatedTime();
long modifiedTime = System.currentTimeMillis();
DatabaseRegistration updatedRegistration =
new DatabaseRegistration(newDescriptor, createdTime,
modifiedTime);
```
--
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]
