tkalkirill commented on code in PR #6673:
URL: https://github.com/apache/ignite-3/pull/6673#discussion_r2394205587
##########
modules/file-io/src/main/java/org/apache/ignite/internal/fileio/AsyncFileIoFactory.java:
##########
@@ -20,14 +20,27 @@
import java.io.IOException;
import java.nio.file.OpenOption;
import java.nio.file.Path;
+import java.util.concurrent.ExecutorService;
/**
* {@link AsyncFileIo} factory.
*/
public class AsyncFileIoFactory implements FileIoFactory {
+ /** Async callback executor. */
+ private final ExecutorService asyncIoExecutor;
Review Comment:
Please note that it can be `null` (with `@Nullable`) and indicate what this
means.
##########
modules/file-io/src/main/java/org/apache/ignite/internal/fileio/AsyncFileIo.java:
##########
@@ -48,11 +49,12 @@ public class AsyncFileIo extends AbstractFileIo {
* Creates I/O implementation for specified file.
*
* @param filePath File path.
+ * @param asyncIoExecutor Callback executor.
Review Comment:
Please note that it can be `null` (with `@Nullable`) and what that means.
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/throttling/PageMemoryThrottlingTest.java:
##########
@@ -135,7 +135,7 @@ void setUp() throws Exception {
FailureManager failureManager = mock(FailureManager.class);
when(failureManager.process(any())).thenThrow(new
AssertionError("Unexpected error"));
- fileIoFactory = spy(new AsyncFileIoFactory());
+ fileIoFactory = spy(new AsyncFileIoFactory(null));
Review Comment:
I propose to get rid of this change.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryStorageEngine.java:
##########
@@ -182,9 +184,26 @@ public void start() throws StorageException {
int pageSize = engineConfig.pageSizeBytes().value();
try {
- FileIoFactory fileIoFactory =
engineConfig.checkpoint().useAsyncFileIoFactory().value()
- ? new AsyncFileIoFactory()
- : new RandomAccessFileIoFactory();
+ FileIoFactory fileIoFactory;
+
Review Comment:
```suggestion
```
##########
modules/file-io/src/main/java/org/apache/ignite/internal/fileio/AsyncFileIo.java:
##########
@@ -48,11 +49,12 @@ public class AsyncFileIo extends AbstractFileIo {
* Creates I/O implementation for specified file.
*
* @param filePath File path.
+ * @param asyncIoExecutor Callback executor.
* @param modes Open modes.
* @throws IOException If some I/O error occurs.
*/
- public AsyncFileIo(Path filePath, OpenOption... modes) throws IOException {
- ch = AsynchronousFileChannel.open(filePath, modes);
+ public AsyncFileIo(Path filePath, ExecutorService asyncIoExecutor,
OpenOption... modes) throws IOException {
+ ch = AsynchronousFileChannel.open(filePath, Set.of(modes),
asyncIoExecutor);
Review Comment:
Please handle the case if `modes` is empty.
##########
modules/page-memory/src/test/java/org/apache/ignite/internal/pagememory/persistence/store/AbstractFilePageStoreIoTest.java:
##########
@@ -405,6 +405,6 @@ void testRenameAndReadRace(FileIoFactory ioFactory) throws
Exception {
}
private static FileIoFactory[] ioFactories() {
- return new FileIoFactory[]{new RandomAccessFileIoFactory(), new
AsyncFileIoFactory()};
+ return new FileIoFactory[]{new RandomAccessFileIoFactory(), new
AsyncFileIoFactory(null)};
Review Comment:
I propose to get rid of this change.
##########
modules/file-io/src/main/java/org/apache/ignite/internal/fileio/AsyncFileIoFactory.java:
##########
@@ -20,14 +20,27 @@
import java.io.IOException;
import java.nio.file.OpenOption;
import java.nio.file.Path;
+import java.util.concurrent.ExecutorService;
/**
* {@link AsyncFileIo} factory.
*/
public class AsyncFileIoFactory implements FileIoFactory {
+ /** Async callback executor. */
+ private final ExecutorService asyncIoExecutor;
+
+ /**
+ * Constructor.
+ *
+ * @param asyncIoExecutor Async callback executor or {@code null} to use
the default pool.
+ */
+ public AsyncFileIoFactory(ExecutorService asyncIoExecutor) {
Review Comment:
I propose creating a default constructor without passing an executor, and
specifying in its documentation that the default executor will be used.
And in this constructor, we can expect not `null`
##########
modules/file-io/src/test/java/org/apache/ignite/internal/fileio/AsyncFileIoTest.java:
##########
@@ -28,7 +28,7 @@
public class AsyncFileIoTest extends AbstractFileIoTest {
@BeforeEach
void setUp() {
- fileIoFactory = new AsyncFileIoFactory();
+ fileIoFactory = new AsyncFileIoFactory(null);
Review Comment:
I propose to get rid of this change.
--
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]
