HIVE-27284: Make HMSHandler proxy pluggable

[wecharyu]

What changes were proposed in this pull request?

1. make HMSHandler Proxy configurable by new metastore conf "metastore.hmshandler.proxy"
2. provide AbstractHMSHandlerProxy to extend customized handler
3. clean some hacked test codes in HMSHandler

Why are the changes needed?

1. make the handler of metastore flexible to config and extend
2. improve the code of HMSHandler

Does this PR introduce any user-facing change?

Yes, user can use new conf and api to extend their own handler.

How was this patch tested?

Passing all existing tests.

[wecharyu]

@deniskuzZ @saihemanth-cloudera @veghlaci05: Could you help review this PR?

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
14 Code Smells

No Coverage information
No Duplication information

[deniskuzZ]

@wecharyu would you be able to rebase?

[deniskuzZ]

do we really need a config for that, since RetryingHMSHandler it the only implementation at this moment?

[wecharyu]

This config will make Meta Store Server more easy to extend other proxy strategy without modifying the main server code, so I highly recommend this config.

[deniskuzZ]

👍

[deniskuzZ]

LGTM, +1 non-binding

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
23 Code Smells

No Coverage information
No Duplication information

[deniskuzZ]

@wecharyu, thank you for the contribution! let's wait approx 1 week for the feedback from @nrg4878, @saihemanth-cloudera, @dengzhhu653.

[deniskuzZ]

+1

[dengzhhu653]

Thanks for the contribution, @wecharyu!
For me, the AbstractHMSHandlerProxy provides an abstract method for his children to have chances to perform some special work on calling proxy, I would prefer a hook for this case, for example, we can introduce a hook like:

interface RetryingHMSHandlerHook {
  default void preCall(proxy, method, args) throws MetaException {
  }
  default void postCall(proxy, method, args, result) {
  }
 // more interfaces if needed....
}

[wecharyu]

@dengzhhu653 Thanks for your review. In this patch we want AbstractHMSHandlerProxy become an interface easy to expand other proxy strategies beyond just "retrying", like timing or something else. It may be different from RetryingHMSHandlerHook, WDYT?

[dengzhhu653]

@dengzhhu653 Thanks for your review. In this patch we want AbstractHMSHandlerProxy become an interface easy to expand other proxy strategies beyond just "retrying", like timing or something else. It may be different from RetryingHMSHandlerHook, WDYT?

Retrying on transient exception is a good way to reduce the network roundtrip client retries, besides there has been a property hive.hmshandler.retry.attempts to control the number of retries, e.g, 0 for disabling the retry mechanism.

Given the template method AbstractHMSHandlerProxy provided:

protected abstract Result invokeInternal(Object proxy, Method method, Object[] args)
      throws Throwable;

Whatever the strategies is, method should is called on the proxy in the child's invokeInternal, that's what RetryingHMSHandler does currently.

So in my view, if there is no special need to introduce complex strategies on method.invoke(proxy, args), then let's keep the codes simple.
Thanks,
Zhihua

[wecharyu]

Yeah, retrying strategy is highly effective in HMS. Actually we just want to refactor the HMSHandler proxy code to be more clear and improve code extensibility. For example we can combine different proxy strategies into one like:

class TimerAndRetryingHMSHandler extends AbstractHMSHandlerProxy {
  private final IHMSHandler retryingHandler; // replace baseHandler
}

[deniskuzZ]

@dengzhhu653, do you have any concerns with the current approach?

[dengzhhu653]

@dengzhhu653, do you have any concerns with the current approach?

I'm +1 on the idea of removing test codes from main routine.
Rather than defining a template method, I'd like to introduce a hook for this purpose, unless we would introduce complex strategies around the method.invoke(proxy, args) in the future.

[deniskuzZ]

@dengzhhu653, do you have any concerns with the current approach?

I'm +1 on the idea of removing test codes from main routine. Rather than defining a template method, I'd like to introduce a hook for this purpose, unless we would introduce complex strategies around the method.invoke(proxy, args) in the future.

I think, regardless if we introduce new custom strategies, the current patch makes the code more readable and extensible/flexible.

[dengzhhu653]

@dengzhhu653, do you have any concerns with the current approach?

I'm +1 on the idea of removing test codes from main routine. Rather than defining a template method, I'd like to introduce a hook for this purpose, unless we would introduce complex strategies around the method.invoke(proxy, args) in the future.

I think, regardless if we introduce new custom strategies, the current patch makes the code more readable and extensible/flexible.

The code itself looks fine to me, though I'm +0 on the factory approach, if we think the benefit wins the complexity and maintenance, I'm fine with the changes.