adoroszlai commented on code in PR #9579:
URL: https://github.com/apache/ozone/pull/9579#discussion_r2681355669
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java:
##########
@@ -123,31 +116,25 @@ protected void loadOMClientConfigs(ConfigurationSource
config, String omSvcId)
*/
@Override
public synchronized ProxyInfo<T> getProxy() {
- ProxyInfo currentProxyInfo =
getOMProxyMap().get(getCurrentProxyOMNodeId());
- if (currentProxyInfo == null) {
- currentProxyInfo = createOMProxy(getCurrentProxyOMNodeId());
- }
- return currentProxyInfo;
+ ProxyInfo<T> current = getOMProxyMap().get(getCurrentProxyOMNodeId());
+ return createOMProxyIfNeeded(current);
}
/**
* Creates proxy object.
*/
- protected ProxyInfo createOMProxy(String nodeId) {
- OMProxyInfo omProxyInfo = omProxyInfos.get(nodeId);
- InetSocketAddress address = omProxyInfo.getAddress();
- ProxyInfo proxyInfo;
- try {
- T proxy = createOMProxy(address);
- // Create proxyInfo here, to make it work with all Hadoop versions.
- proxyInfo = new ProxyInfo<>(proxy, omProxyInfo.toString());
- getOMProxyMap().put(nodeId, proxyInfo);
- } catch (IOException ioe) {
- LOG.error("{} Failed to create RPC proxy to OM at {}",
- this.getClass().getSimpleName(), address, ioe);
- throw new RuntimeException(ioe);
+ protected ProxyInfo<T> createOMProxyIfNeeded(ProxyInfo<T> pi) {
+ if (pi.proxy == null) {
+ OMProxyInfo<T> omProxyInfo = (OMProxyInfo<T>) pi;
Review Comment:
`pi` can be defined as `OMProxyInfo<T>` to avoid the cast.
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProviderBase.java:
##########
@@ -106,19 +121,29 @@ public OMFailoverProxyProviderBase(ConfigurationSource
configuration,
OzoneConfigKeys.OZONE_CLIENT_WAIT_BETWEEN_RETRIES_MILLIS_KEY,
OzoneConfigKeys.OZONE_CLIENT_WAIT_BETWEEN_RETRIES_MILLIS_DEFAULT);
- loadOMClientConfigs(conf, omServiceId);
+ initOmProxiesFromConfigs(conf, omServiceId);
Objects.requireNonNull(omProxies, "omProxies == null");
- Objects.requireNonNull(omNodeIDList, "omNodeIDList == null");
- Objects.requireNonNull(omNodeAddressMap, "omNodeAddressMap == null");
+ Objects.requireNonNull(omNodesInOrder, "omNodesInOrder == null");
+ Preconditions.checkState(omProxies.size() == omNodesInOrder.size(),
+ "omProxies and omNodesInOrder should have the same size");
+ Preconditions.checkState(omProxies.keySet().equals(new
HashSet<>(omNodesInOrder)),
+ "the OM node IDs of omProxies keys should be the same as
omNodesInOrder");
Review Comment:
I prefer `Preconditions` from Ratis, which has `assertSame` and
`assertEquals` methods, and includes expected/actual values in failure message.
Also, these are unnecessary here, since subclasses must set `omProxies` and
`omNodesInOrder` via `initOmProxies`, which also performs the same checks.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
