ivandika3 commented on code in PR #9579:
URL: https://github.com/apache/ozone/pull/9579#discussion_r2682478395
##########
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:
Thanks for the review.
> I prefer Preconditions from Ratis, which has assertSame and assertEquals
methods, and includes expected/actual values in failure message.
Thanks for the info. I agree that it has a better API and I have changed to
use `assertSame` and `assertEquals` from Ratis `Preconditions`.
> Also, these are unnecessary here, since subclasses must set omProxies and
omNodesInOrder via initOmProxies, which also performs the same checks.
I have removed the preconditions from the constructor since it's already
been done in `initOmProxies`.
##########
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:
Thanks for catching this, updated.
--
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]
