[GitHub] [hadoop] touchida commented on a change in pull request #2598: HDFS-15762. TestMultipleNNPortQOP#testMultipleNNPortOverwriteDownStre…

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

touchida commented on a change in pull request #2598:
URL: https://github.com/apache/hadoop/pull/2598#discussion_r554038392



##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestMultipleNNPortQOP.java
##########
@@ -251,62 +252,48 @@ public void testMultipleNNPortOverwriteDownStream() throws Exception {
       clientConf.set(HADOOP_RPC_PROTECTION, "privacy");
       FileSystem fsPrivacy = FileSystem.get(uriPrivacyPort, clientConf);
       doTest(fsPrivacy, PATH1);
-      for (int i = 0; i < 2; i++) {
-        DataNode dn = dataNodes.get(i);
-        SaslDataTransferClient saslClient = dn.getSaslClient();
-        String qop = null;
-        // It may take some time for the qop to populate
-        // to all DNs, check in a loop.
-        for (int trial = 0; trial < 10; trial++) {
-          qop = saslClient.getTargetQOP();
-          if (qop != null) {
-            break;
-          }
-          Thread.sleep(100);
-        }
-        assertEquals("auth", qop);
-      }
+      long count = dataNodes.stream()
+          .map(dn -> dn.getSaslClient().getTargetQOP())
+          .filter(Objects::nonNull).filter(qop -> qop.equals("auth")).count();
+      // For each data pipeline, targetQOPs of sasl clients in the first two
+      // datanodes become equal to auth.
+      // Note that it is not necessarily the case for all datanodes,
+      // since a datanode may be always at the last position in pipelines.
+      assertTrue("At least two qops should be auth", count >= 2);

Review comment:
       @amahussein Thanks for your review!
   Yes, I did.
   But I think we don't have to retry it here, since the test already makes sure that blocks have been replicated.
   https://github.com/apache/hadoop/blob/443a77fbe8dc29e4b36cac7b935405d70bc79158/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestMultipleNNPortQOP.java#L305-L307
   IIUC, qops are populated at sasl negotiation, which should be done before datanodes send packets (i.e., before replication is completed).
   https://github.com/apache/hadoop/blob/443a77fbe8dc29e4b36cac7b935405d70bc79158/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/protocol/datatransfer/sasl/SaslDataTransferClient.java#L414
   
   It seems that the author of the description considered that the flakiness of this test was caused by insufficient sleep time, but I believe it's not.
   https://issues.apache.org/jira/browse/HDFS-15148?focusedCommentId=17026282&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17026282
   
   I also considered to wait for replication inside TestMultipleNNPortQOP#doTest, since FileSystemTestHelper.createFile's success will not necessarily imply replication is completed.
   https://github.com/apache/hadoop/blob/443a77fbe8dc29e4b36cac7b935405d70bc79158/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestMultipleNNPortQOP.java#L301
   But I'm not sure if we really need this.
   May I ask your opinion?




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



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