[#1628] Avoid exception caused by calling release multiple times

[wForget]

What changes were proposed in this pull request?

Set internal buffer to null after release.

Why are the changes needed?

address #1628 (comment)

Fix: #1628

Does this PR introduce any user-facing change?

No.

How was this patch tested?

added unit tests

[rickyma]

Why would release be called multiple times?

[github-actions]

Test Results

 2 641 files  ± 0   2 641 suites  ±0   5h 27m 46s ⏱️ -24s
   946 tests + 2     945 ✅ + 3   1 💤 ±0  0 ❌  - 1 
11 803 runs  +30  11 788 ✅ +31  15 💤 ±0  0 ❌  - 1 

Results for commit d1be2e4. ± Comparison against base commit 1482804.

[wForget]

Why would release be called multiple times?

I didn't find out why it was called multiple times in #1628. Apparently, in

incubator-uniffle/storage/src/main/java/org/apache/uniffle/storage/handler/impl/DataSkippableReadHandler.java

Line 72 in 1482804

if (shuffleIndexResult == null || shuffleIndexResult.isEmpty()) {

, it checks whether shuffleIndexResult is not empty. One of my guesses is that buffer is cleaned up when netty client is closed.

[advancedxy]

Hmm, I'm not sure about this change.

If we can find the root cause why it's called multiple times and can reason that the multiple released is necessary, this change looks good to me.

However, we have not find the root cause. By simply avoid/reset the managedBuffer to null, we may hide a bigger problem and causing more subtle problems later.

Could we add more logging first to help debugging this issue? For example, we can log the double release in the releasing method with the stack trace?

[rickyma]

We need to find out the root cause, which is better. Maybe it has something to do with Uniffle Hadoop related codes. Because this has only happened in RepartitionWithHadoopHybridStorageRssTest.

[wForget]

Thanks, that makes sense to me, I will try uploading GA failure logs for uniffle first.

[rickyma]

Do we have any progress here? :)