goiri commented on code in PR #5651:
URL: https://github.com/apache/hadoop/pull/5651#discussion_r1194137229
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/TestRMFailoverProxyProvider.java:
##########
@@ -303,5 +306,62 @@ public void testAutoRefreshFailoverChange() throws
Exception {
.getProxy(any(YarnConfiguration.class), any(Class.class),
eq(mockAdd3));
}
+
+ @Test
+ public void testRandomSelectRouter() throws Exception {
+
+ // We design a test case like this:
+ // We have three routers (router1, router2, and router3),
+ // we enable Federation mode and random selection mode.
+ // After iterating 50 times, since the selection is random,
+ // each router should be selected more than 0 times,
+ // and the sum of the number of times each router is selected should be
equal to 50.
+
+ final AtomicInteger router1Count = new AtomicInteger(0);
+ final AtomicInteger router2Count = new AtomicInteger(0);
+ final AtomicInteger router3Count = new AtomicInteger(0);
+
+ conf.setBoolean(YarnConfiguration.RM_HA_ENABLED, true);
+ conf.setBoolean(YarnConfiguration.FEDERATION_ENABLED, true);
+
conf.setBoolean(YarnConfiguration.FEDERATION_YARN_CLIENT_FAILOVER_RANDOM_ORDER,
true);
+ conf.set(YarnConfiguration.RM_HA_IDS, "router0,router1,router2");
+
+ // Create two proxies and mock a RMProxy
+ Proxy mockRouterProxy = new TestProxy((proxy, method, args) -> null);
+
+ Class protocol = ApplicationClientProtocol.class;
+ RMProxy<Proxy> mockRMProxy = mock(RMProxy.class);
+ ConfiguredRMFailoverProxyProvider<Proxy> fpp = new
ConfiguredRMFailoverProxyProvider<>();
+
+ // generate two address with different ports.
+ // Default port of yarn RM
+ InetSocketAddress mockRouterAdd = new InetSocketAddress(RM1_PORT);
+
+ // Mock RMProxy methods
+ when(mockRMProxy.getRMAddress(any(YarnConfiguration.class),
+ any(Class.class))).thenReturn(mockRouterAdd);
+ when(mockRMProxy.getProxy(any(YarnConfiguration.class),
+ any(Class.class), eq(mockRouterAdd))).thenReturn(mockRouterProxy);
+
+ // Initialize failover proxy provider and get proxy from it.
+ for (int i = 0; i < NUM_ITERATIONS; i++) {
+ fpp.init(conf, mockRMProxy, protocol);
+ FailoverProxyProvider.ProxyInfo<Proxy> proxy = fpp.getProxy();
+ if (("router0").equals(proxy.proxyInfo)) {
+ router1Count.incrementAndGet();
+ }
+ if (("router1").equals(proxy.proxyInfo)) {
+ router2Count.incrementAndGet();
+ }
+ if (("router2").equals(proxy.proxyInfo)) {
+ router3Count.incrementAndGet();
+ }
+ }
+
+ assertTrue(router1Count.get() < NUM_ITERATIONS && router1Count.get() > 0);
Review Comment:
Split the asserts for < and >
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/TestRMFailoverProxyProvider.java:
##########
@@ -31,6 +31,7 @@
import java.lang.reflect.InvocationHandler;
import java.lang.reflect.Proxy;
import java.net.InetSocketAddress;
+import java.util.concurrent.atomic.AtomicInteger;
import static org.junit.Assert.*;
Review Comment:
As we are at it, expand the import.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/client/ConfiguredRMFailoverProxyProvider.java:
##########
@@ -119,4 +122,31 @@ public synchronized void close() throws IOException {
}
}
}
+
+ /**
+ * YARN Federation mode, the Router is considered as an RM for the client.
+ * We want the client to be able to randomly
+ * Select a Router and support failover when selecting a Router.
+ * The original code always started trying from the first
+ * Router when the client selected a Router,
+ * but this method will support random Router selection.
+ *
+ * For clusters that have not enabled Federation mode, the behavior remains
unchanged.
+ *
+ * @param conf Configuration.
+ * @return rmIds
+ */
+ private Collection<String> getRandomOrder(Configuration conf) {
Review Comment:
The name of function is not correct.
It only returns random if the flag is set.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/TestRMFailoverProxyProvider.java:
##########
@@ -303,5 +306,62 @@ public void testAutoRefreshFailoverChange() throws
Exception {
.getProxy(any(YarnConfiguration.class), any(Class.class),
eq(mockAdd3));
}
+
+ @Test
+ public void testRandomSelectRouter() throws Exception {
+
+ // We design a test case like this:
+ // We have three routers (router1, router2, and router3),
+ // we enable Federation mode and random selection mode.
+ // After iterating 50 times, since the selection is random,
+ // each router should be selected more than 0 times,
+ // and the sum of the number of times each router is selected should be
equal to 50.
+
+ final AtomicInteger router1Count = new AtomicInteger(0);
+ final AtomicInteger router2Count = new AtomicInteger(0);
+ final AtomicInteger router3Count = new AtomicInteger(0);
+
+ conf.setBoolean(YarnConfiguration.RM_HA_ENABLED, true);
+ conf.setBoolean(YarnConfiguration.FEDERATION_ENABLED, true);
+
conf.setBoolean(YarnConfiguration.FEDERATION_YARN_CLIENT_FAILOVER_RANDOM_ORDER,
true);
+ conf.set(YarnConfiguration.RM_HA_IDS, "router0,router1,router2");
+
+ // Create two proxies and mock a RMProxy
+ Proxy mockRouterProxy = new TestProxy((proxy, method, args) -> null);
+
+ Class protocol = ApplicationClientProtocol.class;
+ RMProxy<Proxy> mockRMProxy = mock(RMProxy.class);
+ ConfiguredRMFailoverProxyProvider<Proxy> fpp = new
ConfiguredRMFailoverProxyProvider<>();
+
+ // generate two address with different ports.
+ // Default port of yarn RM
+ InetSocketAddress mockRouterAdd = new InetSocketAddress(RM1_PORT);
+
+ // Mock RMProxy methods
+ when(mockRMProxy.getRMAddress(any(YarnConfiguration.class),
+ any(Class.class))).thenReturn(mockRouterAdd);
+ when(mockRMProxy.getProxy(any(YarnConfiguration.class),
+ any(Class.class), eq(mockRouterAdd))).thenReturn(mockRouterProxy);
+
+ // Initialize failover proxy provider and get proxy from it.
+ for (int i = 0; i < NUM_ITERATIONS; i++) {
+ fpp.init(conf, mockRMProxy, protocol);
+ FailoverProxyProvider.ProxyInfo<Proxy> proxy = fpp.getProxy();
+ if (("router0").equals(proxy.proxyInfo)) {
+ router1Count.incrementAndGet();
+ }
+ if (("router1").equals(proxy.proxyInfo)) {
Review Comment:
Do we need the parenthesis?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/client/ConfiguredRMFailoverProxyProvider.java:
##########
@@ -119,4 +122,31 @@ public synchronized void close() throws IOException {
}
}
}
+
+ /**
+ * YARN Federation mode, the Router is considered as an RM for the client.
+ * We want the client to be able to randomly
+ * Select a Router and support failover when selecting a Router.
+ * The original code always started trying from the first
+ * Router when the client selected a Router,
+ * but this method will support random Router selection.
+ *
+ * For clusters that have not enabled Federation mode, the behavior remains
unchanged.
+ *
+ * @param conf Configuration.
+ * @return rmIds
+ */
+ private Collection<String> getRandomOrder(Configuration conf) {
+ boolean isFederationEnabled = HAUtil.isFederationEnabled(conf);
+ Collection<String> rmIds = HAUtil.getRMHAIds(conf);
+ boolean isRandomOrder = conf.getBoolean(
+ YarnConfiguration.FEDERATION_YARN_CLIENT_FAILOVER_RANDOM_ORDER,
+
YarnConfiguration.DEFAULT_FEDERATION_YARN_CLIENT_FAILOVER_RANDOM_ORDER);
+ if (isFederationEnabled && isRandomOrder) {
Review Comment:
Can we reverse the if and return early if not requested?
--
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]
