So .... why don't we just invoke the callback logic directly after checking if dropped is true or false?

You can see the comment. The catalog contains the authorization plugin. If we drop the catalog first, we can't load the catalog and call the method of the plugin.

Em ... I've rechecked the logic again.
The following logic requires the catalog object anyway, even after this "callback" revision.
The real problem is that we are using the reference to an deleted object to cleanup.
That is pretty dangerous in my viewpoint.
Can we check if we can simply pass object names or IDs, i.e. those primitive types,
to the plugin?

          callAuthorizationPluginImpl(
              authorizationPlugin -> {
                authorizationPlugin.onMetadataUpdated(removeObject);
              },
              catalog);

Actually, this is the cleanup process. Deleting the entity from the store doesn't mean we have deleted the entity. The lifecycle of the catalog object don't end when we just remove it from the store. So I don't think it's dangerous.

So... let's put aside my suggestions on using names/ids for post-deletion operations or similar outcalls.
If the catalog objects are still valid, why don't we just use the objects?

   catalogs = loadCatalogs(ident);
   bSuccess = dropCatalogs(ident, force);
   if (bSuccess) callAuthorizationPlugin(..., catalogs);

It's ok. I can use this implement.

Should we return true directly here?

If not exist, we should return false directly. Changed.

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.

If the catalog doesn't exist, we would return false. Our other code follows this rule, too. Only the catalog exists, we would return true. If we occurs some errors, we would throw exceptions.

Got it. I wasn't suggesting that this is a right or wrong issue.
It is in a gray area, which means we have to cope with it carefully.
We'd better have a clear mindset / protocol on this.

If we occurs some errors, we would throw exceptions.

This, again, leads us to the situation of multiple exit conditions. We may return true/false AND we may throw exceptions. That is not a good design for primitive functions. There are two options that are simpler and clearer:

• Ensure that no exception will be thrown from this function, AND we return a boolean to indicate whether the operation was successful. This means we need to catch the underlying exception(s) and stop propagating them in the call stack.
• Remove the boolean return value. This function either succeeds (silently) or fails with an exception. Hopefully, the exception will be caught very soon before it is causing further misbehaviors.

Thanks for your suggestion. I got your concern. It's a bigger topic. For this pull request, I will the semantic as community discussed before. You can discuss this topic in #5031 if you have concern.

There seems no need to extract this logic as a method.
Thoughts?

I prefer putting the logic into AuthorizationUtils. callAuthorizationPluginImpl is a private implement method.

Em... then why don't we publicize this callAuthorizationPluginImpl?