ayushtkn commented on a change in pull request #4057: URL: https://github.com/apache/hadoop/pull/4057#discussion_r830838309
########## File path: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockReportLease.java ########## @@ -136,6 +137,48 @@ public void testCheckBlockReportLease() throws Exception { } } + @Test + public void testCheckBlockReportLeaseWhenDnUnregister() throws Exception { + HdfsConfiguration conf = new HdfsConfiguration(); + Random rand = new Random(); + + try (MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf) + .numDataNodes(1).build()) { + FSNamesystem fsn = cluster.getNamesystem(); + BlockManager blockManager = fsn.getBlockManager(); + String poolId = cluster.getNamesystem().getBlockPoolId(); + NamenodeProtocols rpcServer = cluster.getNameNodeRpc(); + + // Remove the unique datanode to simulate the unregistered situation. + DataNode dn = cluster.getDataNodes().get(0); + blockManager.getDatanodeManager().getDatanodeMap().remove(dn.getDatanodeUuid()); + + // Trigger BlockReport. + DatanodeRegistration dnRegistration = dn.getDNRegistrationForBP(poolId); + StorageReport[] storages = dn.getFSDataset().getStorageReports(poolId); + ExecutorService pool = Executors.newFixedThreadPool(1); + BlockReportContext brContext = new BlockReportContext(1, 0, + rand.nextLong(), 1); + Future<DatanodeCommand> sendBRfuturea = pool.submit(() -> { Review comment: the variable name is very confusing, I couldn't understand what does a in the end means. ########## File path: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockReportLease.java ########## @@ -136,6 +137,48 @@ public void testCheckBlockReportLease() throws Exception { } } + @Test + public void testCheckBlockReportLeaseWhenDnUnregister() throws Exception { + HdfsConfiguration conf = new HdfsConfiguration(); + Random rand = new Random(); + + try (MiniDFSCluster cluster = new MiniDFSCluster.Builder(conf) + .numDataNodes(1).build()) { Review comment: by default number of datanodes is 1 ########## File path: hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java ########## @@ -2751,6 +2751,11 @@ public boolean checkBlockReportLease(BlockReportContext context, return true; } DatanodeDescriptor node = datanodeManager.getDatanode(nodeID); + if (node == null) { + final UnregisteredNodeException e = new UnregisteredNodeException(nodeID, null); + NameNode.stateChangeLog.error("BLOCK* NameSystem.getDatanode: " + e.getLocalizedMessage()); + throw e; Review comment: Can you share me the log message and the exception trace post this. We have passed null here in the exception. I feel like it can lead to something like: ``Node null is expected to serve this storage`` Which doesn't make sense to me. May be some more appropriate message should be there. -- 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