Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.90.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently multiple clients writing to a RS are serialized when sync'ing their appends. Implementing group commit can help this by being aware of all the clients in that queue.

      1. HBASE-1939-20.4.patch
        31 kB
        Nicolas Spiegelberg
      2. HBASE-1939-v3.patch
        2 kB
        Jean-Daniel Cryans
      3. HBASE-1939-v2.patch
        2 kB
        Jean-Daniel Cryans
      4. HBASE-1939.patch
        15 kB
        Jean-Daniel Cryans

        Issue Links

          Activity

          Hide
          stack added a comment -

          I applied Nicolas's HBASE-1939-20.4.patch above under the aegis of HBASE-2298. It doesn't look like TRUNK has the above issue calling sync (it don't use hdfs-200) but talking J-D in case.

          Show
          stack added a comment - I applied Nicolas's HBASE-1939 -20.4.patch above under the aegis of HBASE-2298 . It doesn't look like TRUNK has the above issue calling sync (it don't use hdfs-200) but talking J-D in case.
          Hide
          Nicolas Spiegelberg added a comment -

          Ported this patch back to version 0.20. Note that this refactoring incidentally fixed a bug on the tip with HLog.sync(). To summarize, the code under sync() should read
          sync(); syncFs()
          instead of
          (syncFs) ? syncFs() : sync();
          sync() flushes the SequenceFile.Writer buffer to DFSOutputStream. syncFs() flushes the output stream across the network to the datanodes. So the current implementation isn't flushing the buffers properly and still gets stuck on local storage until the SequenceFile.Writer buffer is full.

          Show
          Nicolas Spiegelberg added a comment - Ported this patch back to version 0.20. Note that this refactoring incidentally fixed a bug on the tip with HLog.sync(). To summarize, the code under sync() should read sync(); syncFs() instead of (syncFs) ? syncFs() : sync(); sync() flushes the SequenceFile.Writer buffer to DFSOutputStream. syncFs() flushes the output stream across the network to the datanodes. So the current implementation isn't flushing the buffers properly and still gets stuck on local storage until the SequenceFile.Writer buffer is full.
          Hide
          Jean-Daniel Cryans added a comment -

          Resolving since it wasn't the source of Hudson's instability.

          Show
          Jean-Daniel Cryans added a comment - Resolving since it wasn't the source of Hudson's instability.
          Hide
          Jean-Daniel Cryans added a comment -

          I committed without the volatile.

          Show
          Jean-Daniel Cryans added a comment - I committed without the volatile.
          Hide
          Raghu Angadi added a comment -

          yes. should be same as accessing inside a synchronized section. We could consider atomic or volatile if findbugs does not see that.

          Show
          Raghu Angadi added a comment - yes. should be same as accessing inside a synchronized section. We could consider atomic or volatile if findbugs does not see that.
          Hide
          stack added a comment -

          @Raghu Will that ensure all threads see most recent change to forceSync?

          Show
          stack added a comment - @Raghu Will that ensure all threads see most recent change to forceSync?
          Hide
          Raghu Angadi added a comment -

          +1. It does not need to be volatile since it is always accessed under the same lock.

          Show
          Raghu Angadi added a comment - +1. It does not need to be volatile since it is always accessed under the same lock.
          Hide
          stack added a comment -

          Should the forceSync be volatile? Otherwise patch looks good (make it volatile on commit?)

          Show
          stack added a comment - Should the forceSync be volatile? Otherwise patch looks good (make it volatile on commit?)
          Hide
          Jean-Daniel Cryans added a comment -

          v3 with Raghu's comments.

          Show
          Jean-Daniel Cryans added a comment - v3 with Raghu's comments.
          Hide
          Jean-Daniel Cryans added a comment -

          Raghu,

          Again thanks a lot for spending some time looking at my work.

          So the background is that the catalog tables like .META. must never lose any edits so, even if we sync every 100 edits for some reason, when this catalog edit comes in we want to make sure it's safely registered.

          And you are right, this is not the right place to set force as you pointed out.

          Show
          Jean-Daniel Cryans added a comment - Raghu, Again thanks a lot for spending some time looking at my work. So the background is that the catalog tables like .META. must never lose any edits so, even if we sync every 100 edits for some reason, when this catalog edit comes in we want to make sure it's safely registered. And you are right, this is not the right place to set force as you pointed out.
          Hide
          Raghu Angadi added a comment -

          Thanks J-D.

          I should have seen the update earlier... not sure how important 'force' flag is. Currently it might not work properly.

          Say sync thread starts fs_sync() at time t0. At t1 a client appends and invokes a force-sync... I am assuming the contract is it should return after next fs_sync. But in the current implementation, it may not : when fs_sync() returns at t2, it sets this flag to false.. client's force-sync could return without a fs_sync.

          I might implement it something like :

           addToSyncQ(boolean force) {
             lock()
             if (force) {
                  this.forceSync = true;
             }
            //...
          } 

          Rest of the implementation remains same.. with forceSync being a simple boolean rather than an atomic.

          Show
          Raghu Angadi added a comment - Thanks J-D. I should have seen the update earlier... not sure how important 'force' flag is. Currently it might not work properly. Say sync thread starts fs_sync() at time t0. At t1 a client appends and invokes a force-sync... I am assuming the contract is it should return after next fs_sync. But in the current implementation, it may not : when fs_sync() returns at t2, it sets this flag to false.. client's force-sync could return without a fs_sync. I might implement it something like : addToSyncQ( boolean force) { lock() if (force) { this .forceSync = true ; } //... } Rest of the implementation remains same.. with forceSync being a simple boolean rather than an atomic.
          Hide
          Jean-Daniel Cryans added a comment -

          Committed patch to trunk.

          Show
          Jean-Daniel Cryans added a comment - Committed patch to trunk.
          Hide
          stack added a comment -

          +1 Looks good to me.

          Show
          stack added a comment - +1 Looks good to me.
          Hide
          Jean-Daniel Cryans added a comment -

          Second version of the patch against current trunk with Raghu's comments.

          So I changed the condition so that it really sets to force if force == true and forceSync == false. In all the other cases it should stay to current value and hsync resets to false if the value was set to true. I also cleaned a LOG which has an unnecessary stack trace and removed now un-thrown exceptions.

          Show
          Jean-Daniel Cryans added a comment - Second version of the patch against current trunk with Raghu's comments. So I changed the condition so that it really sets to force if force == true and forceSync == false. In all the other cases it should stay to current value and hsync resets to false if the value was set to true. I also cleaned a LOG which has an unnecessary stack trace and removed now un-thrown exceptions.
          Hide
          Jean-Daniel Cryans added a comment -

          Reopening since it seems I was too fast at committing.

          Show
          Jean-Daniel Cryans added a comment - Reopening since it seems I was too fast at committing.
          Hide
          Raghu Angadi added a comment -

          > Wouldn't this leave forceSync to 'true' all the time?
          not all the time. forceSync will remain unchanged when 'force' is true.

          syncDone needs to be signaled in finally? Otherwise might leave the waiters hanging.

          Show
          Raghu Angadi added a comment - > Wouldn't this leave forceSync to 'true' all the time? not all the time. forceSync will remain unchanged when 'force' is true. syncDone needs to be signaled in finally? Otherwise might leave the waiters hanging.
          Hide
          Jean-Daniel Cryans added a comment -

          Raghu,

          Thanks for looking at this patch, I guess I am wrong but in the way you described.

          !(true) && false => false == false then set it to true. That's ok you want to let it true
          !(false) && false => false == false then set it to true. That's not ok we want it to stay false
          !(false) && true => true != false then let it be false. That's not ok we want it to be true
          !(true) && true => false != true then let it be true. That's ok you want it true

          Also that value is reset in hsync to false.

          So I indeed need to fix this thx!

          Show
          Jean-Daniel Cryans added a comment - Raghu, Thanks for looking at this patch, I guess I am wrong but in the way you described. !(true) && false => false == false then set it to true. That's ok you want to let it true !(false) && false => false == false then set it to true. That's not ok we want it to stay false !(false) && true => true != false then let it be false. That's not ok we want it to be true !(true) && true => false != true then let it be true. That's ok you want it true Also that value is reset in hsync to false. So I indeed need to fix this thx!
          Hide
          Raghu Angadi added a comment -

          >+ // Set force sync if force is true and forceSync is false
          >+ forceSync.compareAndSet(!forceSync.get() && force, true);

          Wouldn't this leave forceSync to 'true' all the time?

          Show
          Raghu Angadi added a comment - >+ // Set force sync if force is true and forceSync is false >+ forceSync.compareAndSet(!forceSync.get() && force, true); Wouldn't this leave forceSync to 'true' all the time?
          Hide
          Jean-Daniel Cryans added a comment -

          Committed to trunk, please test your massive inserts and see how better (or worse, could happen) it is when syncing every edit.

          Show
          Jean-Daniel Cryans added a comment - Committed to trunk, please test your massive inserts and see how better (or worse, could happen) it is when syncing every edit.
          Hide
          Jean-Daniel Cryans added a comment -

          Doh missed that part.

          BTW, I cannot have the class static because it relies on the instance of that HLog as it calls hsync and uses its close boolean.

          Show
          Jean-Daniel Cryans added a comment - Doh missed that part. BTW, I cannot have the class static because it relies on the instance of that HLog as it calls hsync and uses its close boolean.
          Hide
          stack added a comment -

          call it hflush? its the new style!

          I did not try your patch. Will do after you commit.

          Show
          stack added a comment - call it hflush? its the new style! I did not try your patch. Will do after you commit.
          Hide
          Jean-Daniel Cryans added a comment -

          I'm good with the comments.

          There's already a method called sync in HLog that's used from HRS and such so I was searching for another name. I'll rename it to syncAppends maybe? Or syncOutputStream?

          Have you tried it on a heavy write job?

          Thx!

          Show
          Jean-Daniel Cryans added a comment - I'm good with the comments. There's already a method called sync in HLog that's used from HRS and such so I was searching for another name. I'll rename it to syncAppends maybe? Or syncOutputStream? Have you tried it on a heavy write job? Thx!
          Hide
          stack added a comment -

          This can be final:

          + private LogSyncer logSyncerThread;

          Can this be static inner class? Paass in flushinterval? And the atomicboolean close? Make it private?

          + public class LogSyncer extends Thread {

          Why syncfs and not just sync? syncfs was name of the method in hdfs-200... its now called hflush?

          Do the above on commit I'd say. They are small changes. Looks good to me J-D.

          Show
          stack added a comment - This can be final: + private LogSyncer logSyncerThread; Can this be static inner class? Paass in flushinterval? And the atomicboolean close? Make it private? + public class LogSyncer extends Thread { Why syncfs and not just sync? syncfs was name of the method in hdfs-200... its now called hflush? Do the above on commit I'd say. They are small changes. Looks good to me J-D.
          Hide
          Jean-Daniel Cryans added a comment -

          Damn forgot to mention that according to my many tests, single thread writing is ~1% slower but using 9 threads you get a 15-20% improvement.

          Show
          Jean-Daniel Cryans added a comment - Damn forgot to mention that according to my many tests, single thread writing is ~1% slower but using 9 threads you get a 15-20% improvement.
          Hide
          Jean-Daniel Cryans added a comment -

          Patch that implements the group commit but that also cleans up HDFS-200 era code. Passes all tests

          • I remove LogFlusher in favor of the LogSyncer thread. This also fixes the fact that the optional log flush value was never use.
          • I use an AtomicBoolean to force sync. This part was tricky because there is no message passing between the client thread and the LogSyncer.
          Show
          Jean-Daniel Cryans added a comment - Patch that implements the group commit but that also cleans up HDFS-200 era code. Passes all tests I remove LogFlusher in favor of the LogSyncer thread. This also fixes the fact that the optional log flush value was never use. I use an AtomicBoolean to force sync. This part was tricky because there is no message passing between the client thread and the LogSyncer.
          Hide
          Andrew Purtell added a comment -

          Sounds good J-D.

          Show
          Andrew Purtell added a comment - Sounds good J-D.
          Hide
          Jean-Daniel Cryans added a comment -

          Well if HBASE-1416 is really just about the first issue then I think this jira plays well with it, we can have a commit thread per WAL for example. Also my patch is already done, I'm currently running the unit tests to make sure I didn't forget something.

          Show
          Jean-Daniel Cryans added a comment - Well if HBASE-1416 is really just about the first issue then I think this jira plays well with it, we can have a commit thread per WAL for example. Also my patch is already done, I'm currently running the unit tests to make sure I didn't forget something.
          Hide
          Andrew Purtell added a comment -

          How does this relate to HBASE-1416? I was looking at that. I think work on that issue and this one would clobber each other.

          Show
          Andrew Purtell added a comment - How does this relate to HBASE-1416 ? I was looking at that. I think work on that issue and this one would clobber each other.

            People

            • Assignee:
              Jean-Daniel Cryans
              Reporter:
              Jean-Daniel Cryans
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development