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

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

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



##########
File path: client/src/main/java/org/apache/crail/metadata/DataNodeStatistics.java
##########
@@ -22,25 +22,29 @@
 import java.nio.ByteBuffer;
 
 public class DataNodeStatistics {
-	public static final int CSIZE = 12;
+	public static final int CSIZE = 14;
 	
 	private long serviceId;
 	private int freeBlockCount;
+	private short status;

Review comment:
       Can you provide possible values for the status? If I understand it correctly it uses RpcErrors.ERR_DATANODE_STOP only? Error seems not to fit to status. Maybe we should define a new class DataNodeStatus where we define the possible status?

##########
File path: client/src/main/java/org/apache/crail/rpc/RpcErrors.java
##########
@@ -55,7 +55,8 @@
 	public static short ERR_DIR_LOCATION_AFFINITY_MISMATCH = 26;
 	public static short ERR_ADD_BLOCK_FAILED = 27;
 	public static short ERR_CREATE_FILE_BUG = 28;
-	
+	public static short ERR_DATANODE_STOP = 29;

Review comment:
       See above.

##########
File path: client/src/main/java/org/apache/crail/rpc/RpcDispatcher.java
##########
@@ -134,6 +135,12 @@ public RpcDispatcher(ConcurrentLinkedQueue<RpcConnection> connectionList) {
 		return connections[0].pingNameNode();
 	}
 
+	@Override
+	public RpcFuture<RpcRemoveDataNode> removeDataNode(
+			InetAddress ipaddr, int port) throws Exception {
+		return connections[0].removeDataNode(ipaddr, port);

Review comment:
       This will not work in a multi-namenode environment. We would have to send a removeDataNode to all Namenodes. For now we should at least check if there is only one namenode and throw an exception otherwise (e.g. datanode removal only possible with one namenode at the moment).

##########
File path: rpc/src/main/java/org/apache/crail/rpc/RpcRequestMessage.java
##########
@@ -571,7 +573,63 @@ public int write(ByteBuffer buffer) {
 		public void update(ByteBuffer buffer) {
 			op = buffer.getInt();
 		}		
-	}	
+	}
+
+	public static class RemoveDataNodeReq implements RpcProtocol.NameNodeRpcMessage {
+		private InetAddress ipAddr;
+		private int port;
+
+		public RemoveDataNodeReq(){
+			this.ipAddr = null;
+			this.port = 0;
+		}
+
+		public RemoveDataNodeReq(InetAddress addr, int port){
+			this.ipAddr = addr;
+			this.port = port;
+		}
+
+		public int size() {
+			//  sizeof(ip-addr) + sizeof(port)
+			return 4 + Integer.BYTES;
+		}
+
+		public short getType(){
+			return RpcProtocol.REQ_REMOVE_DATANODE;
+		}
+
+		public int write(ByteBuffer buffer) throws IOException {
+			int size = size();
+
+			checkSize(buffer.remaining());
+
+			buffer.put(this.getIPAddress().getAddress());
+			buffer.putInt(this.port());
+			return size;
+		}
+
+		private void checkSize(int remaining) throws IOException {
+			if(this.size() > remaining)
+				throw new IOException("Only " + remaining + " remaining bytes stored in buffer, however " + this.size() + " bytes are required");
+		}
+
+		public void update(ByteBuffer buffer) throws IOException {
+			checkSize(buffer.remaining());
+
+			byte[] b = new byte[4];
+			buffer.get(b);
+			this.ipAddr = InetAddress.getByAddress(b);
+			this.port = buffer.getInt();
+		}
+
+		public InetAddress getIPAddress(){

Review comment:
       Minor. Be consistent with the { sometimes there is a space sometimes there isn't. (I prefer space)

##########
File path: rpc/src/main/java/org/apache/crail/rpc/RpcResponseMessage.java
##########
@@ -652,4 +656,49 @@ public void setError(short error) {
 			this.error = error;
 		}
 	}
+
+	public static class RemoveDataNodeRes implements RpcProtocol.NameNodeRpcMessage, RpcRemoveDataNode {
+		public static int CSIZE = Short.BYTES;
+
+		private short data;
+		private short error;
+
+		public RemoveDataNodeRes() {
+			this.data = 0;
+			this.error = 0;
+		}
+
+		public int size() {
+			return CSIZE;
+		}
+
+		public short getType(){
+			return RpcProtocol.RES_REMOVE_DATANODE;
+		}
+
+		public int write(ByteBuffer buffer) {
+			buffer.putShort(data);
+			return CSIZE;
+		}
+
+		public void update(ByteBuffer buffer) {
+			data = buffer.getShort();
+		}
+
+		public short getData(){

Review comment:
       What data? Be a bit more specific. It looks like in https://github.com/apache/incubator-crail/pull/85 it is used as an error.




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