[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3329: Fix data lost when configured multiple ledger directories

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

eolivelli commented on code in PR #3329:
URL: https://github.com/apache/bookkeeper/pull/3329#discussion_r895396529


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -369,8 +369,9 @@ public Checkpoint newCheckpoint() {
                 }
 
                 @Override
-                public void checkpointComplete(Checkpoint checkpoint, boolean compact)
-                        throws IOException {
+                public void checkpointComplete(Checkpoint checkpoint,
+                                               boolean compact,
+                                               LedgerDirsManager ledgerDirsManager1) throws IOException {

Review Comment:
   nit: `ledgerDirsManager`



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -603,7 +604,7 @@ private void replay(Journal journal, JournalScanner scanner) throws IOException
             journalId >= markedLog.getLogFileId());
         // last log mark may be missed due to no sync up before
         // validate filtered log ids only when we have markedLogId
-        if (markedLog.getLogFileId() > 0) {
+        if (markedLog.getLogFileId() > 0 && markedLog.getLogFileId() != Long.MAX_VALUE) {

Review Comment:
   I wonder if it would be better to have a constant for Long.MAX_VALUE



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/CheckpointSource.java:
##########
@@ -89,7 +89,9 @@ public String toString() {
      * @param compact
      *          Flag to compact old checkpoints.
      */
-    void checkpointComplete(Checkpoint checkpoint, boolean compact) throws IOException;
+    void checkpointComplete(Checkpoint checkpoint,
+                            boolean compact,
+                            LedgerDirsManager ledgerDirsManager) throws IOException;

Review Comment:
   we should add javadocs here and explain why we have this `ledgerDirsManager` and when it may be null



##########
tools/perf/src/main/java/org/apache/bookkeeper/tools/perf/journal/JournalWriter.java:
##########
@@ -272,7 +272,7 @@ void execute(Journal[] journals) throws Exception {
             for (Journal journal : journals) {
                 Checkpoint cp = journal.newCheckpoint();
                 try {
-                    journal.checkpointComplete(cp, true);
+                    journal.checkpointComplete(cp, true, null);

Review Comment:
   what about adding a comment here ?
   `null` means that the checkpoint is not started by a specific LedgersDirManager



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -660,7 +660,7 @@ static void writePaddingBytes(JournalChannel jc, ByteBuf paddingBuffer, int jour
     // Should data be fsynced on disk before triggering the callback
     private final boolean syncData;
 
-    private final LastLogMark lastLogMark = new LastLogMark(0, 0);
+    private final LastLogMark lastLogMark = new LastLogMark(Long.MAX_VALUE, Long.MAX_VALUE);

Review Comment:
   we should make a public constant here, otherwise magic numbers are easily forgotten.
   
   also, I may understand why we are changing from 0 to MAX_VALUE, but...what is the impact ?



##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieAccessor.java:
##########
@@ -35,6 +35,6 @@ public static void forceFlush(BookieImpl b) throws IOException {
         CheckpointSourceList source = new CheckpointSourceList(b.journals);
         Checkpoint cp = source.newCheckpoint();
         b.ledgerStorage.flush();
-        source.checkpointComplete(cp, true);
+        source.checkpointComplete(cp, true, null);

Review Comment:
   so `null` means "all"...
   we should document this in the javadocs



-- 
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