[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5569: HDFS-16697.Add code to check the minimumRedundantVolumes value and add related log warning messages.

[via GitHub]

ayushtkn commented on code in PR #5569:
URL: https://github.com/apache/hadoop/pull/5569#discussion_r1198427215


##
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeResourcePolicy.java:
##
@@ -73,6 +78,11 @@ static boolean areResourcesAvailable(
   // required resources available.
   return requiredResourceCount > 0;
 } else {
+  if (minimumRedundantResources > resources.size()){
+LOG.info("Resource not available. Details: redundantResourceCount=" + 
redundantResourceCount
++ ", disabledRedundantResourceCount=" + 
disabledRedundantResourceCount
++ ", minimumRedundantResources=" + minimumRedundantResources + 
".");
+  }
   return redundantResourceCount - disabledRedundantResourceCount >=
   minimumRedundantResources;

Review Comment:
   Change it to 
   ```
   
 final boolean areResourceAvailable =
 redundantResourceCount - disabledRedundantResourceCount >= 
minimumRedundantResources;
 if (!areResourceAvailable) {
   LOG.info("Resources not available. Details: 
redundantResourceCount={},"
   + " disabledRedundantResourceCount={}, 
minimumRedundantResources={}.",
   redundantResourceCount, disabledRedundantResourceCount, 
minimumRedundantResources);
 }
 return areResourceAvailable;
   ```



-- 
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-unsubscr...@hadoop.apache.org

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


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

[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5569: HDFS-16697.Add code to check the minimumRedundantVolumes value and add related log warning messages.

[via GitHub]

ayushtkn commented on code in PR #5569:
URL: https://github.com/apache/hadoop/pull/5569#discussion_r1197581496


##
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeResourcePolicy.java:
##
@@ -73,6 +79,11 @@ static boolean areResourcesAvailable(
   // required resources available.
   return requiredResourceCount > 0;
 } else {
+  if (minimumRedundantResources > resources.size()){
+LOG.warn("The value of " + 
DFSConfigKeys.DFS_NAMENODE_CHECKED_VOLUMES_MINIMUM_KEY

Review Comment:
   this is normal api call and false is genuine response. add a info log not 
warn. And just log resource not available, and put the values of all three 
variables. 
   
   that is the most we can do, rest the admin need to be smart enough to decode 
what value is messed up and what damage it can cause



-- 
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-unsubscr...@hadoop.apache.org

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


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

[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5569: HDFS-16697.Add code to check for minimumRedundantVolumes.

[via GitHub]

ayushtkn commented on code in PR #5569:
URL: https://github.com/apache/hadoop/pull/5569#discussion_r1192914285


##
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeResourceChecker.java:
##
@@ -174,6 +174,19 @@ private void addDirToCheck(URI directoryToCheck, boolean 
required)
* otherwise.
*/
   public boolean hasAvailableDiskSpace() {
+try {
+  if (minimumRedundantVolumes > volumes.size()){
+throw new IllegalArgumentException("The value of "
++ DFSConfigKeys.DFS_NAMENODE_CHECKED_VOLUMES_MINIMUM_KEY
++ " is " + minimumRedundantVolumes
++ " which is greater than the total number of existing storage volumes 
"
++ volumes.size() + " .");
+  }
+} catch (IllegalArgumentException e){
+  LOG.warn("The value of " + 
DFSConfigKeys.DFS_NAMENODE_CHECKED_VOLUMES_MINIMUM_KEY
++ " is greater than the total number of existing storage volumes"
++ " and will result in adding resources and still not being able to 
turn off safe mode.", e);
+}
 return NameNodeResourcePolicy.areResourcesAvailable(volumes.values(),
 minimumRedundantVolumes);

Review Comment:
   minimumRedundantVolumes isn't always an issue if it is more than 
volume.size()
   If you go inside ``areResourcesAvailable``
   this does return true
   ```
   if (redundantResourceCount == 0) {
 // If there are no redundant resources, return true if there are any
 // required resources available.
 return requiredResourceCount > 0;
   }
   ```
   When there is no redundant resource irrespective of 
``minimumRedundantVolumes`` value
   
   Second, if
   
   ```
   // - during startup, if there are no edits dirs on disk, then there is
   // a call to areResourcesAvailable() with no dirs at all, which was
   // previously causing the NN to enter safemode
   if (resources.isEmpty()) {
 return true;
   }
   
   ```
   
   Any place where a log line can be added is here, and that too if it is 
``false``, telling about all these numbers + the total number of values
   ```
   } else {
 return redundantResourceCount - disabledRedundantResourceCount >=
 minimumRedundantResources;
   }
   ```



-- 
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-unsubscr...@hadoop.apache.org

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


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