Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: None
    • Labels:
      None

      Description

      After incorporating v11 of the 2856 fix, we discovered that we are still having some ACID violations.

      This time, however, the problem is not about including "newer" updates; but, about missing older updates
      that should be including.

      Here is what seems to be happening.

      There is a race condition in the StoreScanner.getScanners()

      private List<KeyValueScanner> getScanners(Scan scan,
      final NavigableSet<byte[]> columns) throws IOException {
      // First the store file scanners
      List<StoreFileScanner> sfScanners = StoreFileScanner
      .getScannersForStoreFiles(store.getStorefiles(), cacheBlocks,
      isGet, false);
      List<KeyValueScanner> scanners =
      new ArrayList<KeyValueScanner>(sfScanners.size()+1);

      // include only those scan files which pass all filters
      for (StoreFileScanner sfs : sfScanners) {
      if (sfs.shouldSeek(scan, columns))

      { scanners.add(sfs); }

      }

      // Then the memstore scanners
      if (this.store.memstore.shouldSeek(scan))

      { scanners.addAll(this.store.memstore.getScanners()); }

      return scanners;
      }

      If for example there is a call to Store.updateStorefiles() that happens between
      the store.getStorefiles() and this.store.memstore.getScanners(); then
      it is possible that there was a new HFile created, that is not seen by the
      StoreScanner, and the data is not present in the Memstore.snapshot either.

      1. 4485-v6.diff
        11 kB
        Amitanand Aiyer
      2. 4485-v5.diff
        11 kB
        Amitanand Aiyer
      3. 4485-v4.diff
        9 kB
        Amitanand Aiyer
      4. 4485-v3.diff
        6 kB
        Amitanand Aiyer
      5. 4485-v2.diff
        6 kB
        Amitanand Aiyer
      6. repro_bug-4485.diff
        1.0 kB
        Amitanand Aiyer
      7. 4485-v1.diff
        0.9 kB
        Amitanand Aiyer

        Issue Links

          Activity

          Hide
          Amitanand Aiyer added a comment -

          One way to fix this issue, would be to swap the order in which we clear the snapshot, in Store.updateStoreFiles.

          We might want to notifyReaders() before doing the clearSnapshot(). While it seems like this might fix
          the issue. Here are some issues that it may raise.

          (a) If say Scanner 1 got updated; but Scanner 2 is yet to be updated (waiting for the lock). Scanner 1 may
          now see duplicate data. Some KV's exist both in the new file and in the snapshot.
          Not sure if our current KVHeap mechanism is able to handle this. esp in terms of the number of updates we
          have
          (b) performance concerns about holding on to the snapshot until all the scanners/readers finish scanning
          the columnFamily to allow StoreScanner.updateReaders() to get the lock.

          Show
          Amitanand Aiyer added a comment - One way to fix this issue, would be to swap the order in which we clear the snapshot, in Store.updateStoreFiles. We might want to notifyReaders() before doing the clearSnapshot(). While it seems like this might fix the issue. Here are some issues that it may raise. (a) If say Scanner 1 got updated; but Scanner 2 is yet to be updated (waiting for the lock). Scanner 1 may now see duplicate data. Some KV's exist both in the new file and in the snapshot. Not sure if our current KVHeap mechanism is able to handle this. esp in terms of the number of updates we have (b) performance concerns about holding on to the snapshot until all the scanners/readers finish scanning the columnFamily to allow StoreScanner.updateReaders() to get the lock.
          Hide
          Amitanand Aiyer added a comment -

          v1 is the Initial diff. May have other issues/repurcussions of the change.

          Have not yet tested. Just out there for initial feedback.

          Show
          Amitanand Aiyer added a comment - v1 is the Initial diff. May have other issues/repurcussions of the change. Have not yet tested. Just out there for initial feedback.
          Hide
          Amitanand Aiyer added a comment -

          Here is a way to repro the bug.

          uses unnecessary sleep to get things to go bad. Not intended to be included in the final diff/submission.

          Show
          Amitanand Aiyer added a comment - Here is a way to repro the bug. uses unnecessary sleep to get things to go bad. Not intended to be included in the final diff/submission.
          Hide
          Amitanand Aiyer added a comment -

          fix the issue using locks to ensure that Store.updateStoreFiles does not get called between StoreScanner getting the List of store files, and it getting to the MemStoreScanner.

          Show
          Amitanand Aiyer added a comment - fix the issue using locks to ensure that Store.updateStoreFiles does not get called between StoreScanner getting the List of store files, and it getting to the MemStoreScanner.
          Hide
          Amitanand Aiyer added a comment -

          Move some functions to Store.java so that we do not have to access store.lock from StoreScanner.java

          Passes the TestAcidGuarantees on the internal branch (0.89).

          Running the test suite on the open source trunk (in progress)

          Show
          Amitanand Aiyer added a comment - Move some functions to Store.java so that we do not have to access store.lock from StoreScanner.java Passes the TestAcidGuarantees on the internal branch (0.89). Running the test suite on the open source trunk (in progress)
          Hide
          Amitanand Aiyer added a comment -

          move notifyChangeReaderObservers() outside the lock at all places.

          Testing in progress.

          Show
          Amitanand Aiyer added a comment - move notifyChangeReaderObservers() outside the lock at all places. Testing in progress.
          Hide
          stack added a comment -

          @Amit Great stuff. I like the reasoning above especially the bit where the fix I'd have done, the swapping order, likely has issues.

          Looks like a little pollution in this patch from hbase-4344 but no matter since you've merged this into hbase-4344 over in hbase-4344 (getMaxMemstoreTS?).

          Why move the notify outside of the lock? Is it possible that when done outside of the lock, that observers could ever see different lists of readers?

          Show
          stack added a comment - @Amit Great stuff. I like the reasoning above especially the bit where the fix I'd have done, the swapping order, likely has issues. Looks like a little pollution in this patch from hbase-4344 but no matter since you've merged this into hbase-4344 over in hbase-4344 (getMaxMemstoreTS?). Why move the notify outside of the lock? Is it possible that when done outside of the lock, that observers could ever see different lists of readers?
          Hide
          Jonathan Hsieh added a comment -

          @Amitanand

          I've applied HBASE-2856's from (https://reviews.apache.org/r/2224/diff/#index_header) onto trunk (with minor tweak) and then applied HBASE-4485 but have a compile failure. Specificially matcher.ignoreNewerKVs() seems to be missing. Is there another commit that I'm missing?

          Show
          Jonathan Hsieh added a comment - @Amitanand I've applied HBASE-2856 's from ( https://reviews.apache.org/r/2224/diff/#index_header ) onto trunk (with minor tweak) and then applied HBASE-4485 but have a compile failure. Specificially matcher.ignoreNewerKVs() seems to be missing. Is there another commit that I'm missing?
          Hide
          Ted Yu added a comment -

          Here is the related change w.r.t. ignoreNewerKVs():

          Index: src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java
          ===================================================================
          --- src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java    (revision 1176657)
          +++ src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java    (working copy)
          @@ -59,6 +59,12 @@
             /** Row the query is on */
             protected byte [] row;
          
          +  /** Should we ignore KV's with a newer RWCC timestamp **/
          +  private boolean ignoreNewerKVs = false;
          +  public void ignoreNewerKVs() {
          +    this.ignoreNewerKVs = true;
          +  }
          +
             /**
              * Constructs a ScanQueryMatcher for a Scan.
              * @param scan
          @@ -166,6 +172,12 @@
                   return columns.getNextRowOrNextColumn(bytes, offset, qualLength);
               }
          
          +    // The compaction thread has no readPoint set. For other operations, we
          +    // will ignore updates that are done after the read operation has started.
          +    if (this.ignoreNewerKVs &&
          +        kv.getMemstoreTS() > ReadWriteConsistencyControl.getThreadReadPoint())
          +        return MatchCode.SKIP;
          +
               byte type = kv.getType();
               if (isDelete(type)) {
                 if (tr.withinOrAfterTimeRange(timestamp)) {
          
          Show
          Ted Yu added a comment - Here is the related change w.r.t. ignoreNewerKVs(): Index: src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java (revision 1176657) +++ src/main/java/org/apache/hadoop/hbase/regionserver/ScanQueryMatcher.java (working copy) @@ -59,6 +59,12 @@ /** Row the query is on */ protected byte [] row; + /** Should we ignore KV's with a newer RWCC timestamp **/ + private boolean ignoreNewerKVs = false ; + public void ignoreNewerKVs() { + this .ignoreNewerKVs = true ; + } + /** * Constructs a ScanQueryMatcher for a Scan. * @param scan @@ -166,6 +172,12 @@ return columns.getNextRowOrNextColumn(bytes, offset, qualLength); } + // The compaction thread has no readPoint set. For other operations, we + // will ignore updates that are done after the read operation has started. + if ( this .ignoreNewerKVs && + kv.getMemstoreTS() > ReadWriteConsistencyControl.getThreadReadPoint()) + return MatchCode.SKIP; + byte type = kv.getType(); if (isDelete(type)) { if (tr.withinOrAfterTimeRange(timestamp)) {
          Hide
          Amitanand Aiyer added a comment -

          @Stack. The reason that I wanted to move notifyChangedReaders outside the lock is to avoid a potential race condition where.

          Thread A holds the lock for the Store.java, and wants to do notifyChangedReaders holding the lock.
          NotifyChangedReaders calls updateReaders on a StoreScanner – Say scanner-B

          Thread B is doing a seek on scanner-B so it holds a lock on the StoreScanner object.
          Thread B could now have to call getScanners() (which is now a synchronized function in store) if the heap == null.

          This could end up in a deadlock where Thread A has the lock for Store.java but needs the lock for StoreScanner to get into updateReaders.
          Thread B has the lock for StoreScanner.java but needs the lock for Store.java to get into getScanners and finish the seek().

          Show
          Amitanand Aiyer added a comment - @Stack. The reason that I wanted to move notifyChangedReaders outside the lock is to avoid a potential race condition where. Thread A holds the lock for the Store.java, and wants to do notifyChangedReaders holding the lock. NotifyChangedReaders calls updateReaders on a StoreScanner – Say scanner-B Thread B is doing a seek on scanner-B so it holds a lock on the StoreScanner object. Thread B could now have to call getScanners() (which is now a synchronized function in store) if the heap == null. This could end up in a deadlock where Thread A has the lock for Store.java but needs the lock for StoreScanner to get into updateReaders. Thread B has the lock for StoreScanner.java but needs the lock for Store.java to get into getScanners and finish the seek().
          Hide
          Amitanand Aiyer added a comment -

          I am not clear on weather we need the notifyChangedReader to be under the lock or not? I guess that depends on the
          state that the lock is trying to guard. My understanding/assumption was that

          (a) The lock in Store.java is guarding the list-of-store-files plus the state of MemStore's (kvset and snapshot) references.

          (b) Was also assuming that StoreScanner is okay with seeing an older older set of files, as long as the memStoreScanner that
          it is using is current (i.e. The StoreScanner's view of the world may not be accurate as of "now". But it is guaranteed
          to be consistent across the set of storefiles and the memstore (at the time getScanners was called to create the scanners).

          I don't think that we can totally avoid (b) even if we have notifyChangedReaders under the lock. StoreScanner could already
          be processing a next() operation, during which the updateReader will just have to wait to let the StoreScanner complete, however
          long it takes.

          Please correct me if my assumptions/understanding are wrong.

          Show
          Amitanand Aiyer added a comment - I am not clear on weather we need the notifyChangedReader to be under the lock or not? I guess that depends on the state that the lock is trying to guard. My understanding/assumption was that (a) The lock in Store.java is guarding the list-of-store-files plus the state of MemStore's (kvset and snapshot) references. (b) Was also assuming that StoreScanner is okay with seeing an older older set of files, as long as the memStoreScanner that it is using is current (i.e. The StoreScanner's view of the world may not be accurate as of "now". But it is guaranteed to be consistent across the set of storefiles and the memstore (at the time getScanners was called to create the scanners). I don't think that we can totally avoid (b) even if we have notifyChangedReaders under the lock. StoreScanner could already be processing a next() operation, during which the updateReader will just have to wait to let the StoreScanner complete, however long it takes. Please correct me if my assumptions/understanding are wrong.
          Hide
          Amitanand Aiyer added a comment -

          Btw, Ted also has a fix to this issue where in we use a reader lock and a writer lock in Store.java, and the writerlock is used
          when we update the files. We switch back to the reader lock when we do the notifyChangedReaders.

          We could definitely use that to be "safe". But, I am still trying to understand the case where having notifyChangedReaders
          outside the lock could cause problems.

          Show
          Amitanand Aiyer added a comment - Btw, Ted also has a fix to this issue where in we use a reader lock and a writer lock in Store.java, and the writerlock is used when we update the files. We switch back to the reader lock when we do the notifyChangedReaders. We could definitely use that to be "safe". But, I am still trying to understand the case where having notifyChangedReaders outside the lock could cause problems.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2481/
          -----------------------------------------------------------

          Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg.

          Summary
          -------

          Part of the 2856 diff split into 3 parts for easier review

          The first part is v6 of the patch submitted to:
          https://reviews.apache.org/r/2224/

          This is the fix for HBase-4485

          This addresses bug hbase-4485.
          https://issues.apache.org/jira/browse/hbase-4485

          Diffs


          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c
          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4

          Diff: https://reviews.apache.org/r/2481/diff

          Testing
          -------

          running mvn test with all 3 patches together.

          Thanks,

          Amitanand

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/ ----------------------------------------------------------- Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg. Summary ------- Part of the 2856 diff split into 3 parts for easier review The first part is v6 of the patch submitted to: https://reviews.apache.org/r/2224/ This is the fix for HBase-4485 This addresses bug hbase-4485. https://issues.apache.org/jira/browse/hbase-4485 Diffs src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42 src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4 Diff: https://reviews.apache.org/r/2481/diff Testing ------- running mvn test with all 3 patches together. Thanks, Amitanand
          Hide
          Ted Yu added a comment -

          In Store.completeCompaction(), the following code is outside lock.writeLock:

                // Tell observers that list of StoreFiles has changed.
                notifyChangedReadersObservers();
                // Finally, delete old store files.
                for (StoreFile hsf: compactedFiles) {
                  hsf.deleteReader();
                }
          

          I think the above should be placed in (downgraded) lock.readLock

          Show
          Ted Yu added a comment - In Store.completeCompaction(), the following code is outside lock.writeLock: // Tell observers that list of StoreFiles has changed. notifyChangedReadersObservers(); // Finally, delete old store files. for (StoreFile hsf: compactedFiles) { hsf.deleteReader(); } I think the above should be placed in (downgraded) lock.readLock
          Hide
          Ted Yu added a comment -

          In StoreScanner.useRWCC(boolean flag):

              List<KeyValueScanner> allStoreScanners = 
                this.store.getScanners(cacheBlocks, isGet);
          

          I am not sure if allStoreScanners is a good name - MemStoreScanner is checked later in the method.
          I suggest we keep the original name - scanners.

          Show
          Ted Yu added a comment - In StoreScanner.useRWCC(boolean flag): List<KeyValueScanner> allStoreScanners = this .store.getScanners(cacheBlocks, isGet); I am not sure if allStoreScanners is a good name - MemStoreScanner is checked later in the method. I suggest we keep the original name - scanners.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2481/#review2736
          -----------------------------------------------------------

          Looks good to me. Would be awesome to get this finally out of the way!
          A few nits (like trailing spaces). And some questions below.

          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          <https://reviews.apache.org/r/2481/#comment6164>

          Can this now be a static inner class (if we pass kvSet, etc) to the MemstoreScanner constructor?

          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          <https://reviews.apache.org/r/2481/#comment6166>

          Can these be private?
          volatile because you did not want synchronous in the MemstoreScanner constructor?

          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          <https://reviews.apache.org/r/2481/#comment6162>

          Cool.

          Only concern is that while the scanner exists we may need more memory than before.

          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          <https://reviews.apache.org/r/2481/#comment6160>

          Why can you move this up?

          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          <https://reviews.apache.org/r/2481/#comment6159>

          I'm not usually a fan of instanceof - unless necessary of course.
          Why did you need to fold this into one loop?

          • Lars

          On 2011-10-20 19:13:56, Amitanand Aiyer wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2481/

          -----------------------------------------------------------

          (Updated 2011-10-20 19:13:56)

          Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg.

          Summary

          -------

          Part of the 2856 diff split into 3 parts for easier review

          The first part is v6 of the patch submitted to:

          https://reviews.apache.org/r/2224/

          This is the fix for HBase-4485

          This addresses bug hbase-4485.

          https://issues.apache.org/jira/browse/hbase-4485

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42

          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c

          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4

          Diff: https://reviews.apache.org/r/2481/diff

          Testing

          -------

          running mvn test with all 3 patches together.

          Thanks,

          Amitanand

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/#review2736 ----------------------------------------------------------- Looks good to me. Would be awesome to get this finally out of the way! A few nits (like trailing spaces). And some questions below. src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java < https://reviews.apache.org/r/2481/#comment6164 > Can this now be a static inner class (if we pass kvSet, etc) to the MemstoreScanner constructor? src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java < https://reviews.apache.org/r/2481/#comment6166 > Can these be private? volatile because you did not want synchronous in the MemstoreScanner constructor? src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java < https://reviews.apache.org/r/2481/#comment6162 > Cool. Only concern is that while the scanner exists we may need more memory than before. src/main/java/org/apache/hadoop/hbase/regionserver/Store.java < https://reviews.apache.org/r/2481/#comment6160 > Why can you move this up? src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java < https://reviews.apache.org/r/2481/#comment6159 > I'm not usually a fan of instanceof - unless necessary of course. Why did you need to fold this into one loop? Lars On 2011-10-20 19:13:56, Amitanand Aiyer wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/ ----------------------------------------------------------- (Updated 2011-10-20 19:13:56) Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg. Summary ------- Part of the 2856 diff split into 3 parts for easier review The first part is v6 of the patch submitted to: https://reviews.apache.org/r/2224/ This is the fix for HBase-4485 This addresses bug hbase-4485. https://issues.apache.org/jira/browse/hbase-4485 Diffs ----- src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42 src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4 Diff: https://reviews.apache.org/r/2481/diff Testing ------- running mvn test with all 3 patches together. Thanks, Amitanand
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-10-21 06:13:51, Lars Hofhansl wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java, line 645

          > <https://reviews.apache.org/r/2481/diff/1/?file=51676#file51676line645>

          >

          > Can these be private?

          > volatile because you did not want synchronous in the MemstoreScanner constructor?

          actually, I had this volatile because the kvset in the Memstore is volatile.

          But, on second thought, it seems like we can get rid of that. While the kvset and snapshot in Memstore
          can be accessed from different threads (running different Memstorescanners). The reference in the
          MemstoreScanner should only be used in the Thread that is performing the scan.

          Will update the diff and send it again.

          On 2011-10-21 06:13:51, Lars Hofhansl wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java, line 712

          > <https://reviews.apache.org/r/2481/diff/1/?file=51676#file51676line712>

          >

          > Cool.

          >

          > Only concern is that while the scanner exists we may need more memory than before.

          you are right.

          Are we guaranteed to do a seek() only once? If so, we probably can reset kvset/snapshot to null,
          once we have evaluated kvTail and snapshotTail.

          On 2011-10-21 06:13:51, Lars Hofhansl wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/Store.java, line 1335

          > <https://reviews.apache.org/r/2481/diff/1/?file=51677#file51677line1335>

          >

          > Why can you move this up?

          Here is my understanding/assumption:
          https://issues.apache.org/jira/browse/HBASE-4485?focusedCommentId=13131844&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13131844

          Let me know if that is wrong

          • Amitanand

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2481/#review2736
          -----------------------------------------------------------

          On 2011-10-20 19:13:56, Amitanand Aiyer wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2481/

          -----------------------------------------------------------

          (Updated 2011-10-20 19:13:56)

          Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg.

          Summary

          -------

          Part of the 2856 diff split into 3 parts for easier review

          The first part is v6 of the patch submitted to:

          https://reviews.apache.org/r/2224/

          This is the fix for HBase-4485

          This addresses bug hbase-4485.

          https://issues.apache.org/jira/browse/hbase-4485

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42

          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c

          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4

          Diff: https://reviews.apache.org/r/2481/diff

          Testing

          -------

          running mvn test with all 3 patches together.

          Thanks,

          Amitanand

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-10-21 06:13:51, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java, line 645 > < https://reviews.apache.org/r/2481/diff/1/?file=51676#file51676line645 > > > Can these be private? > volatile because you did not want synchronous in the MemstoreScanner constructor? actually, I had this volatile because the kvset in the Memstore is volatile. But, on second thought, it seems like we can get rid of that. While the kvset and snapshot in Memstore can be accessed from different threads (running different Memstorescanners). The reference in the MemstoreScanner should only be used in the Thread that is performing the scan. Will update the diff and send it again. On 2011-10-21 06:13:51, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java, line 712 > < https://reviews.apache.org/r/2481/diff/1/?file=51676#file51676line712 > > > Cool. > > Only concern is that while the scanner exists we may need more memory than before. you are right. Are we guaranteed to do a seek() only once? If so, we probably can reset kvset/snapshot to null, once we have evaluated kvTail and snapshotTail. On 2011-10-21 06:13:51, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/Store.java, line 1335 > < https://reviews.apache.org/r/2481/diff/1/?file=51677#file51677line1335 > > > Why can you move this up? Here is my understanding/assumption: https://issues.apache.org/jira/browse/HBASE-4485?focusedCommentId=13131844&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13131844 Let me know if that is wrong Amitanand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/#review2736 ----------------------------------------------------------- On 2011-10-20 19:13:56, Amitanand Aiyer wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/ ----------------------------------------------------------- (Updated 2011-10-20 19:13:56) Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg. Summary ------- Part of the 2856 diff split into 3 parts for easier review The first part is v6 of the patch submitted to: https://reviews.apache.org/r/2224/ This is the fix for HBase-4485 This addresses bug hbase-4485. https://issues.apache.org/jira/browse/hbase-4485 Diffs ----- src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42 src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4 Diff: https://reviews.apache.org/r/2481/diff Testing ------- running mvn test with all 3 patches together. Thanks, Amitanand
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-10-21 06:13:51, Lars Hofhansl wrote:

          > src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java, line 216

          > <https://reviews.apache.org/r/2481/diff/1/?file=51678#file51678line216>

          >

          > I'm not usually a fan of instanceof - unless necessary of course.

          > Why did you need to fold this into one loop?

          The reason for having one loop is because we want to get the
          Scanners for the storefiles and the scanners for memstore at
          the same time. This is required for ensuring that the view of
          the StoreScanner is consistent with the state of the store,
          at some point in the execution history.

          I agree, instance of is kinda ugly. Will try to get rid of that.

          • Amitanand

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2481/#review2736
          -----------------------------------------------------------

          On 2011-10-20 19:13:56, Amitanand Aiyer wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/2481/

          -----------------------------------------------------------

          (Updated 2011-10-20 19:13:56)

          Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg.

          Summary

          -------

          Part of the 2856 diff split into 3 parts for easier review

          The first part is v6 of the patch submitted to:

          https://reviews.apache.org/r/2224/

          This is the fix for HBase-4485

          This addresses bug hbase-4485.

          https://issues.apache.org/jira/browse/hbase-4485

          Diffs

          -----

          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42

          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c

          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4

          Diff: https://reviews.apache.org/r/2481/diff

          Testing

          -------

          running mvn test with all 3 patches together.

          Thanks,

          Amitanand

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-10-21 06:13:51, Lars Hofhansl wrote: > src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java, line 216 > < https://reviews.apache.org/r/2481/diff/1/?file=51678#file51678line216 > > > I'm not usually a fan of instanceof - unless necessary of course. > Why did you need to fold this into one loop? The reason for having one loop is because we want to get the Scanners for the storefiles and the scanners for memstore at the same time. This is required for ensuring that the view of the StoreScanner is consistent with the state of the store, at some point in the execution history. I agree, instance of is kinda ugly. Will try to get rid of that. Amitanand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/#review2736 ----------------------------------------------------------- On 2011-10-20 19:13:56, Amitanand Aiyer wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/ ----------------------------------------------------------- (Updated 2011-10-20 19:13:56) Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg. Summary ------- Part of the 2856 diff split into 3 parts for easier review The first part is v6 of the patch submitted to: https://reviews.apache.org/r/2224/ This is the fix for HBase-4485 This addresses bug hbase-4485. https://issues.apache.org/jira/browse/hbase-4485 Diffs ----- src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42 src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4 Diff: https://reviews.apache.org/r/2481/diff Testing ------- running mvn test with all 3 patches together. Thanks, Amitanand
          Hide
          Ted Yu added a comment -

          In order to avoid using instanceof in useRWCC(), we have at least two options:

          1. create a tuple for each KeyValueScanner discovered with boolean flag indicating whether the KeyValueScanner is StoreFileScanner or not.
          Thus scanners would be List<KeyValueScannerWrapper>

          2. Make scanners a Map<KeyValueScanner, Boolean>

          Show
          Ted Yu added a comment - In order to avoid using instanceof in useRWCC(), we have at least two options: 1. create a tuple for each KeyValueScanner discovered with boolean flag indicating whether the KeyValueScanner is StoreFileScanner or not. Thus scanners would be List<KeyValueScannerWrapper> 2. Make scanners a Map<KeyValueScanner, Boolean>
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2481/
          -----------------------------------------------------------

          (Updated 2011-11-17 22:43:10.296972)

          Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg.

          Changes
          -------

          added comments, based on kannan/nicolas' comments on the 89 branch.

          Summary
          -------

          Part of the 2856 diff split into 3 parts for easier review

          The first part is v6 of the patch submitted to:
          https://reviews.apache.org/r/2224/

          This is the fix for HBase-4485

          This addresses bug hbase-4485.
          https://issues.apache.org/jira/browse/hbase-4485

          Diffs (updated)


          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c

          Diff: https://reviews.apache.org/r/2481/diff

          Testing
          -------

          running mvn test with all 3 patches together.

          Thanks,

          Amitanand

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/ ----------------------------------------------------------- (Updated 2011-11-17 22:43:10.296972) Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg. Changes ------- added comments, based on kannan/nicolas' comments on the 89 branch. Summary ------- Part of the 2856 diff split into 3 parts for easier review The first part is v6 of the patch submitted to: https://reviews.apache.org/r/2224/ This is the fix for HBase-4485 This addresses bug hbase-4485. https://issues.apache.org/jira/browse/hbase-4485 Diffs (updated) src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4 src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42 src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c Diff: https://reviews.apache.org/r/2481/diff Testing ------- running mvn test with all 3 patches together. Thanks, Amitanand
          Hide
          Ted Yu added a comment -

          @Amit:
          Can you highlight the changes in the latest patch ?
          There're 12 files where the patch doesn't cleanly apply.

          The patch is much larger than diff-oct-14.diff:

          -rw-r--r--@ 1 zhihyu  110088321  60773 Nov 17 14:51 4485-nov-17.txt
          -rw-r--r--  1 zhihyu  110088321  33693 Oct 14 15:18 4485.v14
          
          Show
          Ted Yu added a comment - @Amit: Can you highlight the changes in the latest patch ? There're 12 files where the patch doesn't cleanly apply. The patch is much larger than diff-oct-14.diff: -rw-r--r--@ 1 zhihyu 110088321 60773 Nov 17 14:51 4485-nov-17.txt -rw-r--r-- 1 zhihyu 110088321 33693 Oct 14 15:18 4485.v14
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2481/
          -----------------------------------------------------------

          (Updated 2011-11-17 23:28:27.463840)

          Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg.

          Changes
          -------

          added comments based on kannan/nicolas' feedback.

          Summary
          -------

          Part of the 2856 diff split into 3 parts for easier review

          The first part is v6 of the patch submitted to:
          https://reviews.apache.org/r/2224/

          This is the fix for HBase-4485

          This addresses bug hbase-4485.
          https://issues.apache.org/jira/browse/hbase-4485

          Diffs (updated)


          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c

          Diff: https://reviews.apache.org/r/2481/diff

          Testing
          -------

          running mvn test with all 3 patches together.

          Thanks,

          Amitanand

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/ ----------------------------------------------------------- (Updated 2011-11-17 23:28:27.463840) Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg. Changes ------- added comments based on kannan/nicolas' feedback. Summary ------- Part of the 2856 diff split into 3 parts for easier review The first part is v6 of the patch submitted to: https://reviews.apache.org/r/2224/ This is the fix for HBase-4485 This addresses bug hbase-4485. https://issues.apache.org/jira/browse/hbase-4485 Diffs (updated) src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 34263e4 src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 7761c42 src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java f5b5c4c Diff: https://reviews.apache.org/r/2481/diff Testing ------- running mvn test with all 3 patches together. Thanks, Amitanand
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/2481/
          -----------------------------------------------------------

          (Updated 2011-11-18 01:35:33.881853)

          Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg.

          Changes
          -------

          rebased with the latest.

          Summary
          -------

          Part of the 2856 diff split into 3 parts for easier review

          The first part is v6 of the patch submitted to:
          https://reviews.apache.org/r/2224/

          This is the fix for HBase-4485

          This addresses bug hbase-4485.
          https://issues.apache.org/jira/browse/hbase-4485

          Diffs (updated)


          src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 747a90b
          src/main/java/org/apache/hadoop/hbase/regionserver/Store.java fec5547
          src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java 6512a54

          Diff: https://reviews.apache.org/r/2481/diff

          Testing
          -------

          running mvn test with all 3 patches together.

          Thanks,

          Amitanand

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2481/ ----------------------------------------------------------- (Updated 2011-11-18 01:35:33.881853) Review request for Ted Yu, Michael Stack, Jonathan Gray, Lars Hofhansl, Amitanand Aiyer, Kannan Muthukkaruppan, Karthik Ranganathan, and Nicolas Spiegelberg. Changes ------- rebased with the latest. Summary ------- Part of the 2856 diff split into 3 parts for easier review The first part is v6 of the patch submitted to: https://reviews.apache.org/r/2224/ This is the fix for HBase-4485 This addresses bug hbase-4485. https://issues.apache.org/jira/browse/hbase-4485 Diffs (updated) src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 747a90b src/main/java/org/apache/hadoop/hbase/regionserver/Store.java fec5547 src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java 6512a54 Diff: https://reviews.apache.org/r/2481/diff Testing ------- running mvn test with all 3 patches together. Thanks, Amitanand
          Hide
          Nicolas Spiegelberg added a comment -

          +1 lgtm. went over this with kannan.

          Show
          Nicolas Spiegelberg added a comment - +1 lgtm. went over this with kannan.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #2454 (See https://builds.apache.org/job/HBase-TRUNK/2454/)
          HBASE-4485 Eliminate window of missing Data

          nspiegelberg :
          Files :

          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #2454 (See https://builds.apache.org/job/HBase-TRUNK/2454/ ) HBASE-4485 Eliminate window of missing Data nspiegelberg : Files : /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java

            People

            • Assignee:
              Amitanand Aiyer
              Reporter:
              Amitanand Aiyer
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development