Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on PR #9905: URL: https://github.com/apache/ozone/pull/9905#issuecomment-4147855230 Thanks for the review @adoroszlai , @Gargi-jais11, @peterxcli, @sreejasahithi, @yandrey321 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
adoroszlai commented on PR #9905: URL: https://github.com/apache/ozone/pull/9905#issuecomment-4142123089 Thanks @rich7420 for the patch, @Gargi-jais11, @peterxcli, @sreejasahithi, @yandrey321 for the review. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
adoroszlai merged PR #9905: URL: https://github.com/apache/ozone/pull/9905 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2975034484
##
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##
@@ -86,22 +99,31 @@ public synchronized boolean read(ByteBuffer buffer) throws
IOException {
* In case of exception, this method catches the exception, logs a warning
message,
* and then continue closing the remaining resources.
*/
+ @Override
public synchronized void close() {
-if (blockFile == null) {
+final File fileToClose = blockFile;
Review Comment:
Oh I think it fixes an existing logging bug. In the original code, blockFile
was set to null before closing the channel, so if an IOException occurred, the
log incorrectly printed null instead of the file name.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2975029064
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final FileChannel failingChannel = mock(FileChannel.class);
+doThrow(new IOException("simulated close
failure")).when(failingChannel).close();
+final RandomAccessFile spyRaf = spy(new RandomAccessFile(f, "rw"));
+setField(c, "blockFile", f);
+setField(c, "channel", failingChannel);
+setField(c, "raf", spyRaf);
+
+assertDoesNotThrow(c::close);
+verify(failingChannel).close();
+verify(spyRaf).close();
+ }
+
+ @Test
+ void closeDoesNotThrowWhenRafAndChannelAreNull() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+setField(c, "blockFile", tempDir.resolve("dummy").toFile());
+setField(c, "channel", null);
+setField(c, "raf", null);
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void readWithZeroSizedBuffer() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("test-file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{1, 2, 3, 4, 5});
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+final ByteBuffer zeroSizedBuffer = ByteBuffer.allocate(0);
+// Should return immediately without reading (buffer has no remaining
capacity)
+assertTrue(c.read(zeroSizedBuffer), "read() should return true for
zero-sized buffer");
+// Verify buffer state unchanged
+assertEquals(0, zeroSizedBuffer.remaining());
+assertEquals(0, zeroSizedBuffer.position());
+assertEquals(0, zeroSizedBuffer.limit());
+
+c.close();
+ }
+
+ @Test
+ void tryWithResourcesClosesAutomatically() throws Exception {
+final File f = tempDir.resolve("try-with-resources").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2975025093
##
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##
@@ -47,9 +48,21 @@ public synchronized boolean isOpen() {
/** Open the given file in read-only mode. */
public synchronized void open(File file) throws FileNotFoundException {
Preconditions.assertNull(blockFile, "blockFile");
-blockFile = Objects.requireNonNull(file, "blockFile == null");
-raf = new RandomAccessFile(blockFile, "r");
-channel = raf.getChannel();
+final File f = Objects.requireNonNull(file, "blockFile == null");
+final RandomAccessFile newRaf = new RandomAccessFile(f, "r");
+try {
+ final FileChannel newChannel = newRaf.getChannel();
Review Comment:
Done. Removed the try-catch block since getChannel() does not throw checked
exceptions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
peterxcli commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2964550778
##
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##
@@ -47,9 +48,21 @@ public synchronized boolean isOpen() {
/** Open the given file in read-only mode. */
public synchronized void open(File file) throws FileNotFoundException {
Preconditions.assertNull(blockFile, "blockFile");
-blockFile = Objects.requireNonNull(file, "blockFile == null");
-raf = new RandomAccessFile(blockFile, "r");
-channel = raf.getChannel();
+final File f = Objects.requireNonNull(file, "blockFile == null");
+final RandomAccessFile newRaf = new RandomAccessFile(f, "r");
+try {
+ final FileChannel newChannel = newRaf.getChannel();
Review Comment:
I think this wont throw, so we can remove the exception handling block
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final FileChannel failingChannel = mock(FileChannel.class);
+doThrow(new IOException("simulated close
failure")).when(failingChannel).close();
+final RandomAccessFile spyRaf = spy(new RandomAccessFile(f, "rw"));
+setField(c, "blockFile", f);
+setField(c, "channel", failingChannel);
+setField(c, "raf", spyRaf);
+
+assertDoesNotThrow(c::close);
+verify(failingChannel).close();
+verify(spyRaf).close();
+ }
+
+ @Test
+ void closeDoesNotThrowWhenRafAndChannelAreNull() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+setField(c, "blockFile", tempDir.resolve("dummy").toFile());
+setField(c, "channel", null);
+setField(c, "raf", null);
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void readWithZeroSizedBuffer() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("test-file").toFile();
+try (RandomAccessFile raf = new Ran
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
peterxcli commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2964559209
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,153 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final FileChannel failingChannel = mock(FileChannel.class);
+doThrow(new IOException("simulated close
failure")).when(failingChannel).close();
+final RandomAccessFile spyRaf = spy(new RandomAccessFile(f, "rw"));
+setField(c, "blockFile", f);
+setField(c, "channel", failingChannel);
+setField(c, "raf", spyRaf);
+
+assertDoesNotThrow(c::close);
+verify(failingChannel).close();
+verify(spyRaf).close();
+ }
+
+ @Test
+ void closeDoesNotThrowWhenRafAndChannelAreNull() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+setField(c, "blockFile", tempDir.resolve("dummy").toFile());
+setField(c, "channel", null);
+setField(c, "raf", null);
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void readWithZeroSizedBuffer() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("test-file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{1, 2, 3, 4, 5});
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+final ByteBuffer zeroSizedBuffer = ByteBuffer.allocate(0);
+// Should return immediately without reading (buffer has no remaining
capacity)
+assertTrue(c.read(zeroSizedBuffer), "read() should return true for
zero-sized buffer");
+// Verify buffer state unchanged
+assertEquals(0, zeroSizedBuffer.remaining());
+assertEquals(0, zeroSizedBuffer.position());
+assertEquals(0, zeroSizedBuffer.limit());
+
+c.close();
+ }
+
+ @Test
+ void tryWithResourcesClosesAutomatically() throws Exception {
+final File f = tempDir.resolve("try-with-resources").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
adoroszlai commented on PR #9905: URL: https://github.com/apache/ozone/pull/9905#issuecomment-4096352700 @peterxcli would you like to take another look? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on PR #9905: URL: https://github.com/apache/ozone/pull/9905#issuecomment-4071804558 @sreejasahithi thanks for the review! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
sreejasahithi commented on code in PR #9905: URL: https://github.com/apache/ozone/pull/9905#discussion_r2941688436 ## hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java: ## Review Comment: nit : Not related to your changes, but there is a typo in javadoc - `ture` should be `true` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
sreejasahithi commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2941741646
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final FileChannel failingChannel = mock(FileChannel.class);
+doThrow(new IOException("simulated close
failure")).when(failingChannel).close();
+final RandomAccessFile spyRaf = spy(new RandomAccessFile(f, "rw"));
+setField(c, "blockFile", f);
+setField(c, "channel", failingChannel);
+setField(c, "raf", spyRaf);
+
+assertDoesNotThrow(c::close);
+verify(spyRaf).close();
+ }
+
+ @Test
+ void closeDoesNotThrowWhenRafAndChannelAreNull() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+setField(c, "blockFile", tempDir.resolve("dummy").toFile());
+setField(c, "channel", null);
+setField(c, "raf", null);
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void readWithZeroSizedBuffer() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("test-file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{1, 2, 3, 4, 5});
+}
+
+c.open(f);
Review Comment:
we should also have c.close()
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
sreejasahithi commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2941688436
##
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##
Review Comment:
Not related to your changes , but there is a typo in javadoc - `ture` should
be `true`
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final FileChannel failingChannel = mock(FileChannel.class);
+doThrow(new IOException("simulated close
failure")).when(failingChannel).close();
+final RandomAccessFile spyRaf = spy(new RandomAccessFile(f, "rw"));
+setField(c, "blockFile", f);
+setField(c, "channel", failingChannel);
+setField(c, "raf", spyRaf);
+
+assertDoesNotThrow(c::close);
+verify(spyRaf).close();
Review Comment:
```suggestion
assertDoesNotThrow(c::close);
verify(failingChannel).close();
verify(spyRaf).close();
```
Optional: We can add `verify(failingChannel).close();` to explicitly verify
the channel close was attempted.
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permiss
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
sreejasahithi commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2941522715
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,150 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final FileChannel failingChannel = mock(FileChannel.class);
+doThrow(new IOException("simulated close
failure")).when(failingChannel).close();
+final RandomAccessFile spyRaf = spy(new RandomAccessFile(f, "rw"));
+setField(c, "blockFile", f);
+setField(c, "channel", failingChannel);
+setField(c, "raf", spyRaf);
+
+assertDoesNotThrow(c::close);
+verify(spyRaf).close();
+ }
+
+ @Test
+ void closeDoesNotThrowWhenRafAndChannelAreNull() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+setField(c, "blockFile", tempDir.resolve("dummy").toFile());
+setField(c, "channel", null);
+setField(c, "raf", null);
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void readWithZeroSizedBuffer() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("test-file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{1, 2, 3, 4, 5});
+}
+
+c.open(f);
Review Comment:
we should also have
c.close()
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2940208511
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,257 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.MappedByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.FileLock;
+import java.nio.channels.ReadableByteChannel;
+import java.nio.channels.WritableByteChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final TrackingRandomAccessFile trackingRaf = new
TrackingRandomAccessFile(f);
+setField(c, "blockFile", f);
+setField(c, "channel", new FailingCloseFileChannel());
+setField(c, "raf", trackingRaf);
+
+assertDoesNotThrow(c::close);
+assertTrue(trackingRaf.isClosed(), "raf.close() should still be called");
+ }
+
+ @Test
+ void closeDoesNotThrowWhenRafAndChannelAreNull() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+setField(c, "blockFile", tempDir.resolve("dummy").toFile());
+setField(c, "channel", null);
+setField(c, "raf", null);
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void readWithZeroSizedBuffer() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("test-file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{1, 2, 3, 4, 5});
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+final ByteBuffer zeroSizedBuffer = ByteBuffer.allocate(0);
+// Should return immediately without reading (buffer has no remaining
capacity)
+assertTrue(c.read(zeroSizedBuffer), "read() should return true for
zero-sized buffer");
+// Verify buffer state unchanged
+assertEquals(0, zeroSizedBuffer.remaining());
+assertEquals(0, zeroSizedBuffer.position());
+assertEquals(0, zeroSizedBuffer.limit());
+ }
+
+ @Test
+ void tryWithResourcesClosesAutomatically() throws Exception {
+final File f = tempDir.resolve("try-with-resources").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{10, 20, 30});
+}
+
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+c.open(f);
+asser
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
peterxcli commented on code in PR #9905:
URL: https://github.com/apache/ozone/pull/9905#discussion_r2923055819
##
hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/io/TestRandomAccessFileChannel.java:
##
@@ -0,0 +1,257 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.utils.io;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.lang.reflect.Field;
+import java.nio.ByteBuffer;
+import java.nio.MappedByteBuffer;
+import java.nio.channels.FileChannel;
+import java.nio.channels.FileLock;
+import java.nio.channels.ReadableByteChannel;
+import java.nio.channels.WritableByteChannel;
+import java.nio.file.Path;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+class TestRandomAccessFileChannel {
+ @TempDir
+ private Path tempDir;
+
+ @Test
+ void openFailureDoesNotLeaveOpenAndCloseIsSafe() {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File missing = tempDir.resolve("missing-file").toFile();
+
+assertThrows(FileNotFoundException.class, () -> c.open(missing));
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeIsIdempotent() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+assertDoesNotThrow(c::close);
+assertFalse(c.isOpen());
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void closeContinuesToCloseRafEvenIfChannelCloseFails() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("file-to-close").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(1);
+}
+
+final TrackingRandomAccessFile trackingRaf = new
TrackingRandomAccessFile(f);
+setField(c, "blockFile", f);
+setField(c, "channel", new FailingCloseFileChannel());
+setField(c, "raf", trackingRaf);
+
+assertDoesNotThrow(c::close);
+assertTrue(trackingRaf.isClosed(), "raf.close() should still be called");
+ }
+
+ @Test
+ void closeDoesNotThrowWhenRafAndChannelAreNull() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+setField(c, "blockFile", tempDir.resolve("dummy").toFile());
+setField(c, "channel", null);
+setField(c, "raf", null);
+assertDoesNotThrow(c::close);
+ }
+
+ @Test
+ void readWithZeroSizedBuffer() throws Exception {
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+final File f = tempDir.resolve("test-file").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{1, 2, 3, 4, 5});
+}
+
+c.open(f);
+assertTrue(c.isOpen());
+
+final ByteBuffer zeroSizedBuffer = ByteBuffer.allocate(0);
+// Should return immediately without reading (buffer has no remaining
capacity)
+assertTrue(c.read(zeroSizedBuffer), "read() should return true for
zero-sized buffer");
+// Verify buffer state unchanged
+assertEquals(0, zeroSizedBuffer.remaining());
+assertEquals(0, zeroSizedBuffer.position());
+assertEquals(0, zeroSizedBuffer.limit());
+ }
+
+ @Test
+ void tryWithResourcesClosesAutomatically() throws Exception {
+final File f = tempDir.resolve("try-with-resources").toFile();
+try (RandomAccessFile raf = new RandomAccessFile(f, "rw")) {
+ raf.write(new byte[]{10, 20, 30});
+}
+
+final RandomAccessFileChannel c = new RandomAccessFileChannel();
+c.open(f);
+asse
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on PR #9905: URL: https://github.com/apache/ozone/pull/9905#issuecomment-4044864327 Thanks so much @adoroszlai , @yandrey321 and @Gargi-jais11 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
adoroszlai commented on PR #9905: URL: https://github.com/apache/ozone/pull/9905#issuecomment-4039759254 @Gargi-jais11 please take a look -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
github-actions[bot] closed pull request #9628: HDDS-14370. RandomAccessFileChannel to implement Closeable URL: https://github.com/apache/ozone/pull/9628 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
github-actions[bot] commented on PR #9628: URL: https://github.com/apache/ozone/pull/9628#issuecomment-4008615433 Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
Gargi-jais11 commented on code in PR #9628:
URL: https://github.com/apache/ozone/pull/9628#discussion_r2762558436
##
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##
@@ -86,22 +90,31 @@ public synchronized boolean read(ByteBuffer buffer) throws
IOException {
* In case of exception, this method catches the exception, logs a warning
message,
* and then continue closing the remaining resources.
*/
+ @Override
public synchronized void close() {
-if (blockFile == null) {
+final File fileToClose = blockFile;
+if (fileToClose == null) {
return;
}
Review Comment:
Check for **null** before copying `blockFile`.
```suggestion
if (blockFile == null) {
return; // Already closed
}
// Store file reference for logging before clearing
final File fileToClose = blockFile;
blockFile = null;
}
```
##
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##
@@ -47,9 +48,12 @@ public synchronized boolean isOpen() {
/** Open the given file in read-only mode. */
public synchronized void open(File file) throws FileNotFoundException {
Preconditions.assertNull(blockFile, "blockFile");
-blockFile = Objects.requireNonNull(file, "blockFile == null");
-raf = new RandomAccessFile(blockFile, "r");
-channel = raf.getChannel();
+final File f = Objects.requireNonNull(file, "blockFile == null");
+final RandomAccessFile newRaf = new RandomAccessFile(f, "r");
+final FileChannel newChannel = newRaf.getChannel();
+blockFile = f;
+raf = newRaf;
+channel = newChannel;
}
Review Comment:
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
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
rich7420 commented on PR #9628: URL: https://github.com/apache/ozone/pull/9628#issuecomment-3742016829 got it! thanks for the review @jojochuang , @Gargi-jais11 ! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
Gargi-jais11 commented on PR #9628: URL: https://github.com/apache/ozone/pull/9628#issuecomment-3741953906 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 r`af.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. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-14370. RandomAccessFileChannel to implement Closeable [ozone]
Gargi-jais11 commented on code in PR #9628:
URL: https://github.com/apache/ozone/pull/9628#discussion_r2684754671
##
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/utils/io/RandomAccessFileChannel.java:
##
Review Comment:
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);
}
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
