Bookkeeper
  1. Bookkeeper
  2. BOOKKEEPER-235

Bad syncing in entrylogger degrades performance for many concurrent ledgers

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1.0
    • Component/s: None
    • Labels:
      None

      Description

      EntryLogger flush syncs on the wrong object, which really hurts performance.

      1. BOOKKEEPER-235.diff
        2 kB
        Ivan Kelly
      2. BOOKKEEPER-235.diff
        1 kB
        Ivan Kelly

        Activity

        Hide
        Ivan Kelly added a comment -

        The problem was that the flush thread was syncing on entryLogger, while EntryLogger#addEntry is also synchronised, so they were holding each other off.

        Fix is two liner.

        Show
        Ivan Kelly added a comment - The problem was that the flush thread was syncing on entryLogger, while EntryLogger#addEntry is also synchronised, so they were holding each other off. Fix is two liner.
        Hide
        Flavio Junqueira added a comment -

        The description refers to EntryLogger flush synchronizing on the wrong object, but the patch changes a synchronization block in InterleavedLedgerStore. For InterleavedLedgerStore flush, it is only called from Bookie SyncThread, and I'm not convinced that it is ok to change the synchronized block like that.

        Show
        Flavio Junqueira added a comment - The description refers to EntryLogger flush synchronizing on the wrong object, but the patch changes a synchronization block in InterleavedLedgerStore. For InterleavedLedgerStore flush, it is only called from Bookie SyncThread, and I'm not convinced that it is ok to change the synchronized block like that.
        Hide
        Ivan Kelly added a comment -

        InterleavedLedgerStorage#flush was syncing on the entrylogger to ensure that only #flush was not called concurrently. It was something i introduced in BOOKKEEPER-196 as an extra check even though it was only called by SyncThread. I didn't realize at the time that EntryLogger#addEntry was also synchronized. Because of this, each call to #addEntry from the bookie thread would block the sync block of #flush in the sync thread.

        Show
        Ivan Kelly added a comment - InterleavedLedgerStorage#flush was syncing on the entrylogger to ensure that only #flush was not called concurrently. It was something i introduced in BOOKKEEPER-196 as an extra check even though it was only called by SyncThread. I didn't realize at the time that EntryLogger#addEntry was also synchronized. Because of this, each call to #addEntry from the bookie thread would block the sync block of #flush in the sync thread.
        Hide
        Flavio Junqueira added a comment -

        InterleavedLedgerStorage#flush essentially calls LedgerCacheImpl#flushLedger and EntryLogger#flush. They both have synchronization in some form. Given that flushLock is kind of dangling in this patch, what if we simply remove the synchronization block in InterleavedLedgerStorage#flush and drop flushLock?

        Show
        Flavio Junqueira added a comment - InterleavedLedgerStorage#flush essentially calls LedgerCacheImpl#flushLedger and EntryLogger#flush. They both have synchronization in some form. Given that flushLock is kind of dangling in this patch, what if we simply remove the synchronization block in InterleavedLedgerStorage#flush and drop flushLock?
        Hide
        Flavio Junqueira added a comment -

        Canceling patch until we converge.

        Show
        Flavio Junqueira added a comment - Canceling patch until we converge.
        Hide
        Ivan Kelly added a comment -

        New patch removes syncing completely, as a) this is only called from SyncThread and b) LedgerCacheImpl#flushLedger and EntryLogger#flush both have their own synchronisation.

        Show
        Ivan Kelly added a comment - New patch removes syncing completely, as a) this is only called from SyncThread and b) LedgerCacheImpl#flushLedger and EntryLogger#flush both have their own synchronisation.
        Hide
        Flavio Junqueira added a comment -

        +1, thanks ivan.

        Show
        Flavio Junqueira added a comment - +1, thanks ivan.
        Hide
        Flavio Junqueira added a comment -

        Committed revision 1336083.

        Show
        Flavio Junqueira added a comment - Committed revision 1336083.
        Hide
        Hudson added a comment -

        Integrated in bookkeeper-trunk #499 (See https://builds.apache.org/job/bookkeeper-trunk/499/)
        BOOKKEEPER-235: Bad syncing in entrylogger degrades performance for many concurrent ledgers (ivank via fpj) (Revision 1336083)

        Result = UNSTABLE
        fpj :
        Files :

        • /zookeeper/bookkeeper/trunk/CHANGES.txt
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
        Show
        Hudson added a comment - Integrated in bookkeeper-trunk #499 (See https://builds.apache.org/job/bookkeeper-trunk/499/ ) BOOKKEEPER-235 : Bad syncing in entrylogger degrades performance for many concurrent ledgers (ivank via fpj) (Revision 1336083) Result = UNSTABLE fpj : Files : /zookeeper/bookkeeper/trunk/CHANGES.txt /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java

          People

          • Assignee:
            Ivan Kelly
            Reporter:
            Ivan Kelly
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development