[GitHub] [hadoop] goiri commented on a diff in pull request #5700: HDFS-17030. Limit wait time for getHAServiceState in ObserverReaderProxy

["goiri (via GitHub)" <gi...@apache.org>]

goiri commented on code in PR #5700:
URL: https://github.com/apache/hadoop/pull/5700#discussion_r1228400308


##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ObserverReadProxyProvider.java:
##########
@@ -155,12 +174,21 @@
    */
   private long observerProbeRetryPeriodMs;
 
+  /**
+   * Timeout in ms when we try to get the HA state of a namenode.
+   */
+  private long namenodeHAStateProbeTimeoutMs;
+
   /**
    * The previous time where zero observer were found. If there was observer,
    * or it is initialization, this is set to 0.
    */
   private long lastObserverProbeTime;
 
+  private final ExecutorService nnProbingThreadPool =
+      new ThreadPoolExecutor(1, 4, 1L, TimeUnit.MINUTES,

Review Comment:
   Lots of magic numbers here.
   They should be constants with names meaning something or at least comments.



##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestObserverReadProxyProvider.java:
##########
@@ -58,30 +74,53 @@
  * NameNode to communicate with.
  */
 public class TestObserverReadProxyProvider {
+  private final static int SLOW_RESPONSE_SLEEP_TIME = 5000; // 5 s

Review Comment:
   Add the unit.
   Probably al use `TimeUnit.SECONDS.toMillis(5)` or the right way.



##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestObserverReadProxyProvider.java:
##########
@@ -58,30 +74,53 @@
  * NameNode to communicate with.
  */
 public class TestObserverReadProxyProvider {
+  private final static int SLOW_RESPONSE_SLEEP_TIME = 5000; // 5 s
+  private final static int NAMENODE_HA_STATE_PROBE_TIMEOUT_SHORT = 2000; // 2s
+  private final static int NAMENODE_HA_STATE_PROBE_TIMEOUT_LONG = 25000; // 25s
 
   private static final LocatedBlock[] EMPTY_BLOCKS = new LocatedBlock[0];
   private String ns;
   private URI nnURI;
-  private Configuration conf;
 
   private ObserverReadProxyProvider<ClientProtocol> proxyProvider;
+  @Mock private Logger logger;
+
   private NameNodeAnswer[] namenodeAnswers;
   private String[] namenodeAddrs;
 
   @Before
   public void setup() throws Exception {
     ns = "testcluster";
     nnURI = URI.create("hdfs://" + ns);
-    conf = new Configuration();
-    conf.set(HdfsClientConfigKeys.DFS_NAMESERVICES, ns);
-    // Set observer probe retry period to 0. Required by the tests that
-    // transition observer back and forth
-    conf.setTimeDuration(
-        OBSERVER_PROBE_RETRY_PERIOD_KEY, 0, TimeUnit.MILLISECONDS);
-    conf.setBoolean(HdfsClientConfigKeys.Failover.RANDOM_ORDER, false);
+
+    MockitoAnnotations.initMocks(this);
+  }
+
+  /**
+   * Replace LOG in ObserverReadProxy with a mocked logger.
+   */
+  private void setupMockLoggerForProxyProvider()

Review Comment:
   Wasn't there a utility to capture logs?
   LogCapturer?
   ```
   LogCapturer logs = GenericTestUtils.LogCapturer.captureLogs(LoggerFactory.getLogger(DataStreamer.class));
   ```



-- 
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: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org