On Fri, Jul 14, 2017 at 6:00 PM, Sijie Guo <guosi...@gmail.com> wrote:
> > > On Sat, Jul 15, 2017 at 8:06 AM, Charan Reddy G <reddychara...@gmail.com> > wrote: > >> Hey, >> >> In InterleavedLedgerStorage, since the initial version of it ( >> https://github.com/apache/bookkeeper/commit/4a94ce1d8184f5f >> 38def015d80777a8113b96690 and https://github.com/apache/book >> keeper/commit/d175ada58dcaf78f0a70b0ebebf489255ae67b5f), addEntry and >> processEntry methods are synchronized. If it is synchronized then I dont >> get what is the point in having 'writeThreadPool' in >> BookieRequestProcessor, if anyhow they are going to be executed >> sequentially because of synchronized addEntry method in >> InterleavedLedgerStorage. >> > > When InterleavedLedgerStore is used in the context of SortedLedgerStore, > the addEntry and processEntry are only called when flushing happened. The > flushing happens in background thread, which is effectively running > sequentially. But adding to the memtable happens concurrently. > > The reason of having 'writeThreadPool' is more on separating writes and > reads into different thread pools. so writes will not be affected by reads. > In the context of SortedLedgerStore, the 'writeThreadPool' adds the > concurrency. > > >> >> If we look at the implementation of addEntry and processEntry method, >> 'somethingWritten' can be made threadsafe by using AtomicBoolean, >> ledgerCache.updateLastAddConfirmed and entryLogger.addEntry methods are >> inherently threadsafe. >> >> I'm not sure about semantics of ledgerCache.putEntryOffset method here. >> I'm not confident enough to say if LedgerCacheImpl and IndexInMemPageMgr >> (and probably IndexPersistenceMgr) are thread-safe classes. >> > > LedgerCacheImpl and IndexInMemPageMgr are thread-safe classes. You can > confirm this from SortedLedgerStorage. > > >> >> As far as I understood, if ledgerCache.putEntryOffset is thread safe, >> then I dont see the need of synchronization for those methods. In any case, >> if they are not thread-safe can you please say why it is not thread-safe >> and how we can do more granular synchronization at LedgerCacheImpl level, >> so that we can remove the need of synchrnoization at >> InterleavedLedgerStorage level. >> > > I don't see any reason why we can't remove the synchronization. > > >> >> I'm currently working on Multiple Entrylogs - >> https://issues.apache.org/jira/browse/BOOKKEEPER-1041. >> > > I am wondering if your multiple-entrylogs approach is making things > complicated. I have been thinking there can be a simpler approach achieving > the same goal: for example, having a ledger storage comprised of N > interleaved/sorted ledger storages, which they share same LedgerCache, but > having different memtables (for sortedledgerstore) and different entry log > files. > This is more cleaner approach. @charan can you comment? JV > > >> To reap the benefits of multipleentrylogs feature from performance >> perspective, this synchrnoization should be taken care or atleast bring it >> down to more granular synchronization (if possible). >> >> @Override >> synchronized public long addEntry(ByteBuffer entry) throws >> IOException { >> long ledgerId = entry.getLong(); >> long entryId = entry.getLong(); >> long lac = entry.getLong(); >> entry.rewind(); >> processEntry(ledgerId, entryId, entry); >> ledgerCache.updateLastAddConfirmed(ledgerId, lac); >> return entryId; >> } >> >> synchronized protected void processEntry(long ledgerId, long entryId, >> ByteBuffer entry, boolean rollLog) >> throws IOException { >> somethingWritten = true; >> long pos = entryLogger.addEntry(ledgerId, entry, rollLog); >> ledgerCache.putEntryOffset(ledgerId, entryId, pos); >> } >> >> Thanks, >> Charan >> > > -- Jvrao --- First they ignore you, then they laugh at you, then they fight you, then you win. - Mahatma Gandhi
