[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3194: BP-50: Add reuse journal file feature to support Intel PMem disk

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

hangc0276 commented on code in PR #3194:
URL: https://github.com/apache/bookkeeper/pull/3194#discussion_r930119988


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##########
@@ -153,23 +153,32 @@ private JournalChannel(File journalDirectory, long logId,
                            long position, boolean fRemoveFromPageCache,
                            int formatVersionToWrite, Journal.BufferedChannelBuilder bcBuilder,
                            ServerConfiguration conf,
-                           FileChannelProvider provider) throws IOException {
+                           FileChannelProvider provider, Long toReplaceLogId) throws IOException {
         this.journalAlignSize = journalAlignSize;
         this.zeros = ByteBuffer.allocate(journalAlignSize);
         this.preAllocSize = preAllocSize - preAllocSize % journalAlignSize;
         this.fRemoveFromPageCache = fRemoveFromPageCache;
         this.configuration = conf;
 
+        boolean reuseFile = false;
         File fn = new File(journalDirectory, Long.toHexString(logId) + ".txn");
+        if (toReplaceLogId != null && logId != toReplaceLogId && provider.supportReuseFile()) {
+            File toReplaceFile = new File(journalDirectory, Long.toHexString(toReplaceLogId) + ".txn");
+            if (toReplaceFile.exists()) {
+                renameJournalFile(toReplaceFile, fn);
+                provider.notifyRename(toReplaceFile, fn);
+                reuseFile = true;
+            }
+        }
         channel = provider.open(fn, configuration);
 
         if (formatVersionToWrite < V4) {
             throw new IOException("Invalid journal format to write : version = " + formatVersionToWrite);
         }
 
         LOG.info("Opening journal {}", fn);
-        if (!channel.fileExists(fn)) { // new file, write version
-            if (!fn.createNewFile()) {
+        if (!channel.fileExists(fn) || reuseFile) { // new file, write version
+            if (!reuseFile && !fn.createNewFile()) {

Review Comment:
   @ArvinDevel  Thanks for your review.
   
   >when the reuseFile is true, we have done renameJournalFile(toReplaceFile, fn), so we expect the channel.fileExists(fn), if not then throw exception, is this alert what you want to ensure?
   
   No, there are two code paths to run into line 180. 
   1) For normal create a new journal file, the `channel.fileExists(fn)` is `false` and `reuseFile` flag is `false`. It will call `createNewFile` and then write the header into the new journal file.
   2) For open an existed journal file to write, it will rename the journal file first. The `channel.fileExists(fn)` is `true` and `reuseFile` is true. It will run into line 181, due to `reuseFile` being `true`, so it won't call `fn.createNewFile` to create a new journal file. In the end, it will write the new header into the journal file to override existing data in the file.
   
   For the test, we need a special FileChannel which records the current written position when rewriting an existing journal file. It is a little hard to run into a reuseFile case.



-- 
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: issues-unsubscribe@bookkeeper.apache.org

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