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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
