[#4236] feat(core): Supports the post-hook for the managers or dispatcher#4239
[#4236] feat(core): Supports the post-hook for the managers or dispatcher#4239xunliu merged 3 commits intoapache:mainfrom
Conversation
roryqi
force-pushed
the
ISSUE-4236
branch
5 times, most recently
from
d36f6d1 to
feef232
Compare
|
@jerryshao Could you take a look? |
|
@xunliu Could you help me review this pull request? |
|
|
||
| // Provides a universal entrance to install lifecycle hooks. This method | ||
| // focuses the logic of installing hooks. | ||
| // We should reuse the ability of *NormalizeDispatcher to avoid solving |
There was a problem hiding this comment.
Which mean is *NormalizeDispatcher, Typo?
*NormalizeDispatcher*?
There was a problem hiding this comment.
NO. It represents TableNormalizeDispatcher,SchemaNormalizeDispatcher, etc.
There was a problem hiding this comment.
I think better change to We should reuse the ability of (Metalake|Schema|Table|Fileset|...)NormalizeDispatcher to avoid solving
| namespace); | ||
| } | ||
|
|
||
| public static <T> void prepareAuthorizationHooks(T manager, LifecycleHooks hooks) { |
There was a problem hiding this comment.
I think better to add some comments for this function.
| } | ||
|
|
||
| public static <T> void prepareAuthorizationHooks(T manager, LifecycleHooks hooks) { | ||
| if (manager instanceof SupportsMetalakes) { |
There was a problem hiding this comment.
This method only hooks the event of createXXXX?
| public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { | ||
| Object result = method.invoke(dispatcher, args); | ||
| List<BiConsumer> postHooks = hooks.getPostHooks(method.getName()); | ||
| for (BiConsumer hook : postHooks) { |
There was a problem hiding this comment.
What if anyone fails halfway through?
| import java.util.List; | ||
| import java.util.function.BiConsumer; | ||
|
|
||
| class LifecycleHookProxy<T> implements InvocationHandler { |
There was a problem hiding this comment.
Perhaps it's better to name the class DispatcherHookProxy
There was a problem hiding this comment.
Em.. all is ok for me. But many system use life cycle hook when they use similar mechinansim like K8S, Android.
There was a problem hiding this comment.
I also feeling DispatcherHookProxy as a better name.
3 participants
What changes were proposed in this pull request?
Supports the post-hook for the managers or dispatcher.
For example, after we create a securable object, we should set an owner for it. We can add a specific post hook to support it.
This pull request uses Java dynamic proxy mechanism to support this feature. We only support post-hook now, we can support pre-hook in the future.
Why are the changes needed?
Fix: #4236
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Add a new ut.