laserninja commented on code in PR #10699:
URL: https://github.com/apache/gravitino/pull/10699#discussion_r3047904637
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/CatalogWrapperForREST.java:
##########
@@ -130,6 +131,18 @@ public LoadTableResponse loadTable(
return loadTableResponse;
}
+ public LoadTableResponse registerTable(
+ Namespace namespace, RegisterTableRequest request, boolean
requestCredential) {
+ LoadTableResponse loadTableResponse = super.registerTable(namespace,
request);
+ if (shouldGenerateCredential(loadTableResponse, requestCredential)) {
+ return injectCredentialConfig(
+ TableIdentifier.of(namespace, request.name()),
+ loadTableResponse,
+ CredentialPrivilege.WRITE);
Review Comment:
`createTable` hardcodes `WRITE` — that makes sense since the client just
created the table and likely needs to write data to it. But `registerTable` is
semantically different: the table data already exists and the client is just
adding it to the catalog. The typical next action after registration is
**reading** the data. Meanwhile, `loadTable` accepts the privilege as a
parameter and delegates the decision to the caller.
Should `registerTable` similarly accept/infer the privilege, or is the
`WRITE` default intentional here? At minimum, a brief comment explaining the
choice would help future readers.
--
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]
