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/4a94ce1d8184f5f38def015d80777a > 8113b96690 and https://github.com/apache/bookkeeper/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. > 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 >
