[GitHub] [incubator-crail] PepperJo commented on a change in pull request #85: Resource Elasticity [2/3]

[GitBox <gi...@apache.org>]

PepperJo commented on a change in pull request #85:
URL: https://github.com/apache/incubator-crail/pull/85#discussion_r577773163



##########
File path: namenode/src/main/java/org/apache/crail/namenode/BlockStore.java
##########
@@ -84,7 +84,28 @@ public DataNodeBlocks getDataNode(DataNodeInfo dnInfo) {
 		int storageClass = dnInfo.getStorageClass();
 		return storageClasses[storageClass].getDataNode(dnInfo);
 	}
-	
+
+	short removeDataNode(DataNodeInfo dn) throws Exception {

Review comment:
       We should be careful with `throws Exception` but instead be more specific on the exception type, e.g. IOException or similar

##########
File path: client/src/main/java/org/apache/crail/tools/RemoveDataNode.java
##########
@@ -0,0 +1,87 @@
+package org.apache.crail.tools;
+
+import org.apache.commons.cli.*;
+import org.apache.crail.conf.CrailConfiguration;
+import org.apache.crail.conf.CrailConstants;
+import org.apache.crail.rpc.*;
+import org.apache.crail.utils.CrailUtils;
+import org.slf4j.Logger;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Arrays;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.Future;
+
+public class RemoveDataNode {
+
+    RemoveDataNode() {}
+
+    public static void main(String[] args) throws Exception {
+
+        InetAddress ipaddr = null;
+        int port = -1;
+
+        Option ipOption = Option.builder("i").desc("Ip address").hasArg().build();
+        Option portOption = Option.builder("p").desc("port").hasArg().build();
+
+        Options options = new Options();
+        options.addOption(ipOption);
+        options.addOption(portOption);
+
+        CommandLineParser parser = new DefaultParser();
+        CommandLine line = parser.parse(options, Arrays.copyOfRange(args, 0, args.length));

Review comment:
       You can just do `line = parser.parse(options, args);`

##########
File path: client/src/main/java/org/apache/crail/tools/RemoveDataNode.java
##########
@@ -0,0 +1,87 @@
+package org.apache.crail.tools;
+
+import org.apache.commons.cli.*;
+import org.apache.crail.conf.CrailConfiguration;
+import org.apache.crail.conf.CrailConstants;
+import org.apache.crail.rpc.*;
+import org.apache.crail.utils.CrailUtils;
+import org.slf4j.Logger;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Arrays;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.Future;
+
+public class RemoveDataNode {
+
+    RemoveDataNode() {}
+
+    public static void main(String[] args) throws Exception {
+
+        InetAddress ipaddr = null;
+        int port = -1;
+
+        Option ipOption = Option.builder("i").desc("Ip address").hasArg().build();
+        Option portOption = Option.builder("p").desc("port").hasArg().build();
+
+        Options options = new Options();
+        options.addOption(ipOption);
+        options.addOption(portOption);
+
+        CommandLineParser parser = new DefaultParser();
+        CommandLine line = parser.parse(options, Arrays.copyOfRange(args, 0, args.length));
+
+        if(line.hasOption(ipOption.getOpt())) {
+            ipaddr = InetAddress.getByName(line.getOptionValue(ipOption.getOpt()));
+        } else {
+            throw new Exception("Missing Ip-Address specification");

Review comment:
       This is more an FYI, i.e. it is not a stopper for approval of this PR. Just something I prefer: Instead of checking if it exists you can add `.required()` to the option: `Option ipOption = Option.builder("i").desc("Ip address").hasArg().required().build()` and catch the error when calling `.parse` and print the automatic help with: 
   ```
   HelpFormatter formatter = new HelpFormatter();
   formatter.printHelp("RemoveDataNode", options);
   ```

##########
File path: namenode/src/main/java/org/apache/crail/namenode/BlockStore.java
##########
@@ -171,9 +192,33 @@ short addDataNode(DataNodeBlocks dataNode) {
 		return RpcErrors.ERR_OK;
 
 	}
-	
+
+	short prepareForRemovalDatanode(DataNodeInfo dn) throws Exception {
+		// this will only mark it for removal
+		return prepareOrRemoveDN(dn, true);
+	}
+
+	short removeDatanode(DataNodeInfo dn) throws Exception {
+		// this will remove it as well
+		return prepareOrRemoveDN(dn, false);
+	}
+
 	//---------------
-	
+
+	private short prepareOrRemoveDN(DataNodeInfo dn, boolean onlyMark) throws Exception {
+		DataNodeBlocks toBeRemoved = membership.get(dn.key());
+		if (toBeRemoved == null) {
+			LOG.error("DataNode: " + dn.toString() + " not found");
+			return RpcErrors.ERR_DATANODE_NOT_REGISTERED;
+		} else {
+			if (onlyMark)
+				toBeRemoved.scheduleForRemoval();

Review comment:
       please use `{` also for single line if/else. It should be like this elsewhere.

##########
File path: namenode/src/main/java/org/apache/crail/namenode/BlockStore.java
##########
@@ -171,9 +192,33 @@ short addDataNode(DataNodeBlocks dataNode) {
 		return RpcErrors.ERR_OK;
 
 	}
-	
+
+	short prepareForRemovalDatanode(DataNodeInfo dn) throws Exception {

Review comment:
       `throws Exception`

##########
File path: client/src/main/java/org/apache/crail/tools/RemoveDataNode.java
##########
@@ -0,0 +1,87 @@
+package org.apache.crail.tools;
+
+import org.apache.commons.cli.*;
+import org.apache.crail.conf.CrailConfiguration;
+import org.apache.crail.conf.CrailConstants;
+import org.apache.crail.rpc.*;
+import org.apache.crail.utils.CrailUtils;
+import org.slf4j.Logger;
+
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.util.Arrays;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.Future;
+
+public class RemoveDataNode {
+
+    RemoveDataNode() {}

Review comment:
       Not need if empty except you want to change visibility

##########
File path: namenode/src/main/java/org/apache/crail/namenode/BlockStore.java
##########
@@ -171,9 +192,33 @@ short addDataNode(DataNodeBlocks dataNode) {
 		return RpcErrors.ERR_OK;
 
 	}
-	
+
+	short prepareForRemovalDatanode(DataNodeInfo dn) throws Exception {
+		// this will only mark it for removal
+		return prepareOrRemoveDN(dn, true);
+	}
+
+	short removeDatanode(DataNodeInfo dn) throws Exception {
+		// this will remove it as well
+		return prepareOrRemoveDN(dn, false);
+	}
+
 	//---------------
-	
+
+	private short prepareOrRemoveDN(DataNodeInfo dn, boolean onlyMark) throws Exception {
+		DataNodeBlocks toBeRemoved = membership.get(dn.key());
+		if (toBeRemoved == null) {
+			LOG.error("DataNode: " + dn.toString() + " not found");
+			return RpcErrors.ERR_DATANODE_NOT_REGISTERED;
+		} else {
+			if (onlyMark)
+				toBeRemoved.scheduleForRemoval();
+			else
+				membership.remove(toBeRemoved.key());

Review comment:
       Can you explain what happens when we do not mark but just remove?

##########
File path: namenode/src/main/java/org/apache/crail/namenode/BlockStore.java
##########
@@ -84,7 +84,28 @@ public DataNodeBlocks getDataNode(DataNodeInfo dnInfo) {
 		int storageClass = dnInfo.getStorageClass();
 		return storageClasses[storageClass].getDataNode(dnInfo);
 	}
-	
+
+	short removeDataNode(DataNodeInfo dn) throws Exception {
+		int storageClass = dn.getStorageClass();
+		return storageClasses[storageClass].removeDatanode(dn);
+	}
+
+	short prepareDataNodeForRemoval(DataNodeInfo dn) throws Exception {

Review comment:
       `throws Exception` see above.

##########
File path: namenode/src/main/java/org/apache/crail/namenode/BlockStore.java
##########
@@ -171,9 +192,33 @@ short addDataNode(DataNodeBlocks dataNode) {
 		return RpcErrors.ERR_OK;
 
 	}
-	
+
+	short prepareForRemovalDatanode(DataNodeInfo dn) throws Exception {
+		// this will only mark it for removal
+		return prepareOrRemoveDN(dn, true);
+	}
+
+	short removeDatanode(DataNodeInfo dn) throws Exception {
+		// this will remove it as well
+		return prepareOrRemoveDN(dn, false);
+	}
+
 	//---------------
-	
+
+	private short prepareOrRemoveDN(DataNodeInfo dn, boolean onlyMark) throws Exception {

Review comment:
       `throws Exception`

##########
File path: namenode/src/main/java/org/apache/crail/namenode/BlockStore.java
##########
@@ -171,9 +192,33 @@ short addDataNode(DataNodeBlocks dataNode) {
 		return RpcErrors.ERR_OK;
 
 	}
-	
+
+	short prepareForRemovalDatanode(DataNodeInfo dn) throws Exception {
+		// this will only mark it for removal
+		return prepareOrRemoveDN(dn, true);
+	}
+
+	short removeDatanode(DataNodeInfo dn) throws Exception {

Review comment:
       `throws Exception`




----------------------------------------------------------------
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:
users@infra.apache.org