[GitHub] [kafka] divijvaidya commented on a diff in pull request #12331: KAFKA-1194: changes needed to run on Windows

[GitBox <gi...@apache.org>]

divijvaidya commented on code in PR #12331:
URL: https://github.com/apache/kafka/pull/12331#discussion_r912790203


##########
core/src/main/scala/kafka/log/AbstractIndex.scala:
##########
@@ -108,32 +107,42 @@ abstract class AbstractIndex(@volatile private var _file: File, val baseOffset:
   @volatile
   protected var mmap: MappedByteBuffer = {
     val newlyCreated = file.createNewFile()
-    val raf = if (writable) new RandomAccessFile(file, "rw") else new RandomAccessFile(file, "r")
+
+    val options = if (writable) List(StandardOpenOption.READ, StandardOpenOption.WRITE, StandardOpenOption.SPARSE) else List(StandardOpenOption.READ, StandardOpenOption.SPARSE)

Review Comment:
   You don't need StandardOpenOption.SPARSE for read-only mode since that option is only applicable as a hint to operating system when creating new files. see: https://docs.oracle.com/javase/7/docs/api/java/nio/file/StandardOpenOption.html#SPARSE 



##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -1432,4 +1425,21 @@ public static String[] enumOptions(Class<? extends Enum<?>> enumClass) {
                 .toArray(String[]::new);
     }
 
+    /**
+     * Creates a preallocated file

Review Comment:
   Please add a note that the responsibility to free up resource i.e. close the FileChannel is transferred to the caller of this function.



##########
clients/src/main/java/org/apache/kafka/common/utils/Utils.java:
##########
@@ -1432,4 +1425,21 @@ public static String[] enumOptions(Class<? extends Enum<?>> enumClass) {
                 .toArray(String[]::new);
     }
 
+    /**
+     * Creates a preallocated file
+     * @param path File path
+     * @param size The size used for pre allocate file, for example 512 * 1025 *1024
+     */
+    public static FileChannel createPreallocatedFile(Path path, int size) throws IOException {

Review Comment:
   My suggestion:
   1. Create another function preallocatedFile() which will not create a new file but instead it will just preallocate the file i.e. it won't have `StandardOpenOption.CREATE_NEW`
   2. Refactor the code such that there is no code duplication between `createPreallocatedFile` and  `preallocatedFile`.
   3. You can now use this new `preallocatedFile` function to replace AbstractIndex.scala, lines 121-126 and lines 195-200.



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org