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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
