@Sijie..Did you get chance to go through the scenario/code path. @JV..will create a bug, once I get clarity.
Thanks, Charan On Fri, Oct 13, 2017 at 5:19 PM, Venkateswara Rao Jujjuri <jujj...@gmail.com > wrote: > Charan this looks like an issue to me. Do we have a defect/issue opened? > > On Fri, Oct 13, 2017 at 4:18 PM, Sijie Guo <guosi...@gmail.com> wrote: > > > Charan, > > > > Didn't mean to say the logic is correct. I was just trying to point out > > the points that I remembered for checkpoint. > > > > I am currently traveling, so I don't have code available to check the > > sequence. I can check the logic when I am close to the laptop. > > > > Sijie > > > > > > On Oct 14, 2017 6:11 AM, "Charan Reddy G" <reddychara...@gmail.com> > wrote: > > > > Hey Sijie, > > > > I'm not questioning the semantics of checkpoint or the optimization which > > was added with Bookkeeper-564. But my concern is are we sure, checkpoint > > logic/code is correct and "marker is only updated when all the entries > > added before are persisted.", in the case of SortedLedgerStorage. Can you > > please go through the scenario I mentioned in my email. From what I > > understood, if entryLog is rotated because of addEntry request from GC, > > then we set the 'currentMark' of journal as 'lastCheckpoint' in > > checkpointHolder. The entries added before this 'lastCheckpoint' are > still > > in EntryMemTable. When next checkpoint happens, we are not actually > > persisting entries which were in EntryMemTable but we are marking > > 'lastCheckpoint' as LogMarkCheckpoint (in checkpointComplete method). > > > > Thanks, > > Charan > > > > On Fri, Oct 13, 2017 at 2:46 PM, Sijie Guo <guosi...@gmail.com> wrote: > > > > > The core of the checkpoint is: > > > > > > - marker is only updated when all the entries added before are > persisted. > > > That means it doesn't affect correctness if entries added after are > > > flushed. > > > > > > - the flush in entry log files is just writing data to filesystem. The > > real > > > fsync happens after checkpoint. The separate is for performance > > > consideration. > > > > > > > > > > > > On Oct 12, 2017 11:34 PM, "Charan Reddy G" <reddychara...@gmail.com> > > > wrote: > > > > > > > Hey Sijie/IvanK, > > > > > > > > With > > > > https://github.com/apache/bookkeeper/commit/ > > > d175ada58dcaf78f0a70b0ebebf489 > > > > 255ae67b5f > > > > you introduced Bookkeeper-564 : Better checkpoint mechanism - > > Scheduling > > > > checkpoint only when rotating an entry log file. > > > > > > > > I'm trying to understand how it would work in the following scenario > > > > - using SortedLedgerStorage > > > > - since it is SortedLedgerStorage entries would be in EntryMemtable > > > > - GarbageCollectorThread.EntryLogScanner.process method calls > > > entryLogger > > > > .addEntry(ledgerId, entry) > > > > - in EntryLogger.addEntry method, lets say it comes to know it has > > > reached > > > > EntryLogLimit and creates NewLog > > > > - since current active entrylog is rotated, > > > > EntryLogListener.onRotateEntryLog is called > > > > - which sets the currentMark of journal to checkpointHolder. Point to > > > note, > > > > that all the entries added to the Bookie are not added to entryLog > yet, > > > > there are entries still in entrymemtable > > > > - lets say SyncThread tries to checkpoint at this instant > > > > > > > > now the concern is, in SortedLedgerStorage.checkpoint method, before > > > > calling super.checkpoint(checkpoint), it does memTable.flush(this, > > > > checkpoint); But memTable.flush would just add entries to the current > > > > active entrylog (BufferedLogChannel) and it doesn't guarantee > > > persistence. > > > > super(InterLeavedLedgerStorage).checkpoint will only > flushRotatedLogs > > > > (persists) and finally mark the checkpointcomplete with > > 'lastcheckpoint', > > > > but the 'lastCheckpoint' in the checkpointHolder would also include > the > > > > entries which were in Entrymemtable and are not actually persisted in > > the > > > > whole process. Is there issue in SortedLedgerStorage checkpoint > logic? > > > > > > > > @Override > > > > public Checkpoint checkpoint(final Checkpoint checkpoint) throws > > > > IOException { > > > > Checkpoint lastCheckpoint = checkpointHolder. > > > getLastCheckpoint(); > > > > // if checkpoint is less than last checkpoint, we don't need > to > > > do > > > > checkpoint again. > > > > if (lastCheckpoint.compareTo(checkpoint) > 0) { > > > > return lastCheckpoint; > > > > } > > > > memTable.flush(this, checkpoint); > > > > return super.checkpoint(checkpoint); > > > > } > > > > > > > > Thanks, > > > > Charan > > > > > > > > > > > > > > > > -- > Jvrao > --- > First they ignore you, then they laugh at you, then they fight you, then > you win. - Mahatma Gandhi >
