HDDS-14370. RandomAccessFileChannel to implement Closeable

[rich7420]

What changes were proposed in this pull request?

The class already had a close() method but didn't implement Closeable, which could lead to resource leaks if developers miss calling close().

What is the link to the Apache JIRA

HDDS-14370

How was this patch tested?

https://github.com/rich7420/ozone/actions/runs/20941407661

[Gargi-jais11]

Thanks @rich7420 for the patch.
Please see the inline comment.

[Gargi-jais11]

This is not related to your changes but I found a bug over here.
Here, blockFile is set to null before it was used in the catch blocks
for error logging. This would cause log messages to show null instead of the actual file path when closing fails making debugging difficult.
Solution: Save the file reference so that error messages can properly display the file path.

public synchronized void close() {
    if (blockFile == null) {
      return;
    }
    final File fileToClose = blockFile;
    blockFile = null;
    try {
       channel.close();
       channel = null;
    } catch (IOException e) {
      LOG.warn("Failed to close channel for {}", fileToClose, e);
    }
    try {
     raf.close();
     raf = null;
    } catch (IOException e) {
      LOG.warn("Failed to close RandomAccessFile for {}", fileToClose, e);
    }
  }
}

[jojochuang]

Better to have a test file for this class: TestRandomAccessFileChannel

[Gargi-jais11]

Below the few test cases suggestion which I could think as of now:

1. Exception during close() — partial resource cleanup

• Test: If channel.close() throws IOException, verify raf.close() is still called
• Test: If raf.close() throws IOException, verify channel.close() was already attempted
• Rationale: close() catches exceptions but must attempt to close both resources even if one fails to prevent file descriptor leaks.

2. Partial initialization failure

• Test: Simulate open() failing after setting blockFile but before raf/channel are set
• Verify: close() handles null raf/channel gracefully (null checks exist, but should be tested)
• Rationale: If open() throws mid-initialization, close() must handle partial state safely.

3. Zero-sized buffer

• Test: read() with ByteBuffer.allocate(0) — buffer has no remaining capacity
• Expected: Should return immediately without reading; verify return value
• Rationale: Edge case that could cause infinite loops or incorrect behavior.

It would be great @rich7420 if you come up with some more critical tests.

[rich7420]

got it! thanks for the review @jojochuang , @Gargi-jais11 !

[Gargi-jais11]

Please find the inline comments

[Gargi-jais11]

Check for null before copying blockFile.

Suggested change

	final File fileToClose = blockFile;
	if (fileToClose == null) {
	return;
	}
	if (blockFile == null) {
	return; // Already closed
	}
	// Store file reference for logging before clearing
	final File fileToClose = blockFile;
	blockFile = null;
	}

[Gargi-jais11]

I think we should consider wrapping open() in try-catch to clean up on failure .
I am saying this as I suggest that :
If FileNotFoundException is thrown: blockFile is set, but raf and channel remain null.
Here the problem is that isOpen() returns true (because blockFile != null), but the object isn't usable. And will the subsequent calls to read() or position() will fail with NPE when accessing channel