tengqm commented on code in PR #6045:
URL: https://github.com/apache/gravitino/pull/6045#discussion_r1900893202
##########
core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java:
##########
@@ -126,8 +126,19 @@ public boolean dropCatalog(NameIdentifier ident) {
@Override
public boolean dropCatalog(NameIdentifier ident, boolean force)
throws NonEmptyEntityException, CatalogInUseException {
- AuthorizationUtils.authorizationPluginRemovePrivileges(ident,
Entity.EntityType.CATALOG);
- return dispatcher.dropCatalog(ident, force);
+ // If we call the authorization plugin after dropping catalog, we can't
load the plugin of the
+ // catalog
+ Catalog catalog = null;
+ if (dispatcher.catalogExists(ident)) {
+ catalog = dispatcher.loadCatalog(ident);
+ }
Review Comment:
Emm ... think twice about the protocol please.
This function returns a boolean. You can use it for one of two purposes:
- an indication of whether the `dropCatalog` operation was successful;
- an indication of whether the catalog (and related stuff) no longer exists.
Both are okay.
I'm more inclined to the second pattern and the reason is that I would expect
each function to serve a business logic with a business purpose.
This is a hint I got from line 141, where the function returns `dropped` as
a boolean.
To be consistent, I'd see a negative return value as catalog not dropped.
This is not a mandatory change request. I was just dumping my brain on the
(functional) interface design. It is still okay to return `false` in this
case,
which means the return value is about whether the operation was successful
or not.
--
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]
