This is a regression of HDFS-7915.

From what I can see, the assertion failed when the expected number of slots is 2, not 1.

Assigning this JIRA to me. I found it is due to race condition. The test checks the number of slots before the slots are removed.

Rev01: changed checkNumberOfSegmentsAndSlots. Instead of assert the number of slots right away, use GenericTestUtils.waitFor() helper method to periodically check the number of slots. The time out of GenericTestUtils.waitFor is set to infinity, but the test case it self has a predefined timeout, so it shouldn't matter.

-1 overall

This message was automatically generated.

Test failures appear unrelated.
I've run the test locally and had no failures in 1000 runs. Previously the test fails frequently, about 1 in 5 runs.

Thanks Wei-Chiu Chuang for working on this. The analysis sounds reasonable to me. I have 1 question and 2 minor comments.
Question:
Do we know why this failed only for testDataXceiverCleansUpSlotsOnFailure, but not for similar checks in testDataXceiverHandlesRequestShortCircuitShmFailure or testPreReceiptVerificationDfsClientCanDoScr? I personally only saw testDataXceiverCleansUpSlotsOnFailure fail intermittently, but never met the others. I could've missed them, but just want to understand better.
Comments:

• Can we update testPreReceiptVerificationDfsClientCanDoScr to use the extracted method checkNumberOfSegmentsAndSlots?
• Can we still set an estimated max for waitFor? The test case has its own timeouts, but IMHO wait for Integer.MAX_VALUE is not best practice.

Thank you very much for the review and comments!
(1)
AFAIK, the three tests use different fault injectors. testDataXceiverCleansUpSlotsOnFailure injects a failure at BlockReaderFactory.requestFileDescriptors(); while testDataXceiverHandlesRequestShortCircuitShmFailure injects a failure at DataXceiver.sendShmSuccessResponse, and testPreReceiptVerificationDfsClientCanDoScr injects a failure at BlockReaderFactory.requestFileDescriptors().

(2) testPreReceiptVerificationDfsClientCanDoScr should also call checkNumberOfSegmentsAndSlots to maintain consistency.

(3) Definitely. I was just not sure what would be a reasonable number. Setting 1 second or 10 seconds should be more than sufficient though.

Rev02: set a time out of 10 seconds. use checkNumberOfSegmentsAndSlots() in testPreReceiptVerificationDfsClientCanDoScr() to check numbers of segments and slots.

-1 overall

This message was automatically generated.

Thanks Wei-Chiu. +1 (non-binding).

Thanks for looking at this, Wei-Chiu Chuang. The idea behind this test is that the slots should never be removed by a timeout. That's why this line is there:

    conf.setLong(
        HdfsClientConfigKeys.Read.ShortCircuit.STREAMS_CACHE_EXPIRY_MS_KEY,
        1000000000L);

Unless I'm missing something, adding a waitFor will not have any effect here, since the streams expiry timeout is longer than the test timeout. I'm not completely sure why we're getting 2 slots here instead of 1... we might need to see if we can get log files from a test run that failed.

Hi Colin P. McCabe,
Thanks for looking into this patch.
IIUC, the patch is trying to fix the race where the check happens before the slot is removed before the failed call (The following is injected to be failing). So the expiry should be fine, and the waitFor is just for testing purpose on the check. Wei-Chiu please correct me if I'm wrong.

    // The second read should fail, and we should only have 1 segment and 1 slot
    // left.
    fs.getClient().getConf().brfFailureInjector =
        new TestCleanupFailureInjector();
    try {
      DFSTestUtil.readFileBuffer(fs, TEST_PATH2);
    } catch (Throwable t) {
      GenericTestUtils.assertExceptionContains("TCP reads were disabled for " +
          "testing, but we failed to do a non-TCP read.", t);
    }

Colin P. McCabe Xiao is right about what I thought. It does appear there is a race. From your perspective, do you think that's by design, or some unintended bugs in the code?

Thanks for the reviews!

Hmm. Can you be clearer on what the race condition is here?

Attaching the log of a failed run.

Colin P. McCabe sorry for the long delay ...

The hypothesis here is that after TestCleanupFailureInjector#injectRequestFileDescriptorsFailure throws an exception to inject failure, it takes some time to propagate that exception, and we occasionally test and verify if the number of slots is one, before it catches the exception and remove the slot. This race between removing the slot and checking the slot size failed the test.

Adding a watiFor seem to remove the race.

Hello Colin P. McCabe, would you mind to review it again? Thank you very much!

Thanks for the explanation. It sounds like the race condition is that the ShortCircuitRegistry on the DN needs to be informed about the client's decision that short-circuit is not working for the block, and this RPC takes time to arrive. That background process races with completing the TCP read successfully and checking the number of slots in the unit test.

   public static interface Visitor {
-    void accept(HashMap<ShmId, RegisteredShm> segments,
+    boolean accept(HashMap<ShmId, RegisteredShm> segments,
                 HashMultimap<ExtendedBlockId, Slot> slots);
   }

I don't think it makes sense to change the return type of the visitor. While you might find a boolean convenient, some other potential users of the interface might not find it useful. Instead, just have your closure modify a final MutableBoolean declared nearby.

+    }, 100, 10000);

It seems like we could lower the latency here (perhaps check every 10 ms) and lengthen the timeout. Since the test timeouts are generally 60s, I don't think it makes sense to make this timeout shorter than that.

+1 once that's addressed. Thanks, Wei-Chiu Chuang. Sorry for the delay in reviews.

SUCCESS: Integrated in Hadoop-trunk-Commit #9891 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9891/)
HDFS-9466. TestShortCircuitCache#testDataXceiverCleansUpSlotsOnFailure (cmccabe: rev c7921c9bddb79c9db5059b6c3f7a3a586a3cd95b)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/ShortCircuitRegistry.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/shortcircuit/TestShortCircuitCache.java

Committed to 2.8. Thanks, Wei-Chiu Chuang.