[ 
https://issues.apache.org/jira/browse/HDFS-15920?focusedWorklogId=573376&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-573376
 ]

ASF GitHub Bot logged work on HDFS-15920:
-----------------------------------------

                Author: ASF GitHub Bot
            Created on: 29/Mar/21 09:25
            Start Date: 29/Mar/21 09:25
    Worklog Time Spent: 10m 
      Work Description: ayushtkn commented on a change in pull request #2831:
URL: https://github.com/apache/hadoop/pull/2831#discussion_r603138171



##########
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManagerSafeMode.java
##########
@@ -640,7 +643,17 @@ private void reportStatus(String msg, boolean rightNow) {
    */
   private class SafeModeMonitor implements Runnable {
     /** Interval in msec for checking safe mode. */
-    private static final long RECHECK_INTERVAL = 1000;
+    private long recheckInterval = 1000;
+
+    public SafeModeMonitor() {
+      Configuration conf = new HdfsConfiguration();

Review comment:
       No need to create a new `conf` object, get the conf object passed on 
from `BlockManagerSafeMode` constructor and initialise the daemon inside the 
constructor itself.
   ```
       smmthread = new Daemon(new SafeModeMonitor(conf));
   ```
   
   

##########
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManagerSafeMode.java
##########
@@ -366,6 +368,22 @@ public void testSafeModeMonitor() throws Exception {
     assertFalse(bmSafeMode.isInSafeMode());
   }
 
+  @Test
+  public void testSafemodeRecheckIntervalValue() {
+    Configuration conf = new HdfsConfiguration();
+    long interval = conf.getLong(
+            DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_KEY,
+            DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_DEFAULT);
+
+    assertEquals(interval, DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_DEFAULT);
+
+    conf.setLong(DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_KEY, 2000);
+    interval = conf.getLong(
+            DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_KEY,
+            DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_DEFAULT);
+    assertEquals(interval, 2000);
+  }

Review comment:
       What is this test testing? The working of configuration class? That 
isn't required? What you need to test is the namenode is using the configured 
value as the recheck interval.

##########
File path: 
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManagerSafeMode.java
##########
@@ -640,7 +643,17 @@ private void reportStatus(String msg, boolean rightNow) {
    */
   private class SafeModeMonitor implements Runnable {
     /** Interval in msec for checking safe mode. */
-    private static final long RECHECK_INTERVAL = 1000;
+    private long recheckInterval = 1000;
+
+    public SafeModeMonitor() {
+      Configuration conf = new HdfsConfiguration();
+      long interval = conf.getLong(
+              DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_KEY,
+              DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_DEFAULT);
+      if (interval > 0) {
+        recheckInterval = interval;

Review comment:
       Add a warn log telling the configured value is invalid and you are using 
the default value.
   
   Secondly as of now you are having two variables, `recheckInterval` & 
`interval`, Keep only `recheckInterval` and initialise it in the constructor, 
if the value is greater than 0, no issues, if it is not set it to 
`DFS_NAMENODE_SAFEMODE_RECHECK_INTERVAL_DEFAULT`




-- 
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.

For queries about this service, please contact Infrastructure at:
[email protected]


Issue Time Tracking
-------------------

    Worklog Id:     (was: 573376)
    Time Spent: 20m  (was: 10m)

> Solve the problem that the value of SafeModeMonitor#RECHECK_INTERVAL can be 
> configured
> --------------------------------------------------------------------------------------
>
>                 Key: HDFS-15920
>                 URL: https://issues.apache.org/jira/browse/HDFS-15920
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>            Reporter: JiangHua Zhu
>            Assignee: JiangHua Zhu
>            Priority: Major
>              Labels: pull-request-available
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> The current SafeModeMonitor#RECHECK_INTERVAL value has a fixed value (=1000), 
> and this value should be set and configurable. Because the lock is occupied 
> internally, it competes with other places.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to