[feature] Support alter database comment and custom properties#1172
[feature] Support alter database comment and custom properties#1172LiebingYu wants to merge 2 commits intoapache:mainfrom
Conversation
d88c76c to
434a4f4
Compare
98ba457 to
c3292f4
Compare
c3292f4 to
ae84bc6
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for ALTER DATABASE to update a database’s comment and custom properties across the RPC API, server coordinator, Java client admin, and Flink catalog integration (linked to #1151).
Changes:
- Introduces a new RPC API (
ALTER_DATABASE) with request/response messages and gateway wiring. - Implements server-side handling to apply database property changes and persist them in ZooKeeper.
- Adds client- and Flink-side implementations plus new unit/integration tests for altering database properties/comments.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| fluss-server/src/test/java/org/apache/fluss/server/coordinator/TestCoordinatorGateway.java | Extends test gateway interface with alterDatabase stub. |
| fluss-server/src/main/java/org/apache/fluss/server/zk/ZooKeeperClient.java | Adds ZK helper to update an existing database znode payload. |
| fluss-server/src/main/java/org/apache/fluss/server/utils/ServerRpcMessageUtils.java | Adds PbAlterConfig → DatabaseChange conversions (including comment special-casing). |
| fluss-server/src/main/java/org/apache/fluss/server/entity/DatabasePropertyChanges.java | New internal representation of database property/comment changes. |
| fluss-server/src/main/java/org/apache/fluss/server/coordinator/MetadataManager.java | Adds coordinator logic to apply and persist database changes. |
| fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorService.java | Adds alterDatabase RPC handler + conversion to DatabasePropertyChanges. |
| fluss-rpc/src/main/proto/FlussApi.proto | Defines AlterDatabaseRequest/Response messages. |
| fluss-rpc/src/main/java/org/apache/fluss/rpc/protocol/ApiKeys.java | Adds new ALTER_DATABASE API key. |
| fluss-rpc/src/main/java/org/apache/fluss/rpc/gateway/AdminGateway.java | Exposes alterDatabase on the admin RPC gateway. |
| fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/sink/testutils/TestAdminAdapter.java | Updates test adapter interface surface with alterDatabase stub. |
| fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/catalog/FlinkCatalogTest.java | Adds catalog-level unit coverage for altering database props/comments. |
| fluss-flink/fluss-flink-common/src/test/java/org/apache/fluss/flink/catalog/FlinkCatalogITCase.java | Adds SQL-based IT coverage for ALTER DATABASE ... SET (...). |
| fluss-flink/fluss-flink-common/src/main/java/org/apache/fluss/flink/catalog/FlinkCatalog.java | Implements Flink Catalog#alterDatabase by diffing descriptors into DatabaseChanges. |
| fluss-common/src/main/java/org/apache/fluss/metadata/DatabaseChange.java | New public change model for database alters (set/reset/updateComment). |
| fluss-common/src/main/java/org/apache/fluss/config/FlussConfigUtils.java | Adds shared constant COMMENT_PROP_NAME = "comment". |
| fluss-client/src/test/java/org/apache/fluss/client/admin/FlussAdminITCase.java | Adds IT coverage for admin-side alter database flows. |
| fluss-client/src/main/java/org/apache/fluss/client/utils/ClientRpcMessageUtils.java | Adds DatabaseChange → PbAlterConfig conversion for database alters. |
| fluss-client/src/main/java/org/apache/fluss/client/admin/FlussAdmin.java | Implements Admin#alterDatabase RPC call construction and dispatch. |
| fluss-client/src/main/java/org/apache/fluss/client/admin/Admin.java | Adds new public alterDatabase API to the Admin interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fluss-server/src/main/java/org/apache/fluss/server/coordinator/MetadataManager.java
Outdated
Show resolved
Hide resolved
fluss-server/src/main/java/org/apache/fluss/server/coordinator/MetadataManager.java
Outdated
Show resolved
Hide resolved
fluss-server/src/main/java/org/apache/fluss/server/entity/DatabasePropertyChanges.java
Show resolved
Hide resolved
| customPropertiesToReset.add(key); | ||
| } | ||
|
|
||
| public void setComment(String comment) { |
There was a problem hiding this 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".
| public void setComment(String comment) { | |
| public void setComment(@Nullable String comment) { |
|
|
||
| @Override | ||
| public String toString() { | ||
| return "SetComment{" + "comment='" + comment + '\'' + '}'; |
There was a problem hiding this 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).
| return "SetComment{" + "comment='" + comment + '\'' + '}'; | |
| return "UpdateComment{" | |
| + (comment == null ? "clearComment" : "comment='" + comment + '\'') | |
| + '}'; |
loserwang1024
left a comment
There was a problem hiding this comment.
Thanks for your job, I have left some comment.
| } catch (Exception e) { | ||
| Throwable t = ExceptionUtils.stripExecutionException(e); | ||
| if (CatalogExceptionUtils.isDatabaseNotExist(t)) { | ||
| if (!ignoreIfNotExists) { |
There was a problem hiding this comment.
admin.alterDatabase(databaseName, databaseChanges, ignoreIfNotExists) already handle ignoreIfNotExists. Thus, just throw exception here
There was a problem hiding this comment.
admin.alterDatabase throws org.apache.fluss.exception.DatabaseNotExistException while FlinkCatalog.alterDatabase throws org.apache.flink.table.catalog.exceptions.DatabaseNotExistException. We need to wrap it here like dropDatabase does.
| } | ||
|
|
||
| // alter database request and response | ||
| message AlterDatabaseRequest { |
There was a problem hiding this comment.
where is comment? I think comment should be seperated from config_changes, @wuchong , WDYT?
There was a problem hiding this comment.
I think we can treat comment as a specific config. Otherwise we should extract comment from List<DatabaseChange> databaseChanges which is a bit of a hassle.
b1f92c5 to
ac67a7e
Compare
Purpose
Linked issue: close #1151
Brief change log
Tests
API and Format
Documentation