Details

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

      Description

      HBASE-10572 adds a test that does 'get' from client. We should add a similar test for batch gets.

      1. 0025-HBASE-10616.-Addendum-that-fixes-the-case-of-multige.patch
        1 kB
        Enis Soztutar
      2. 0023-HBASE-10616.-Integration-test-for-multi-get-calls.patch
        19 kB
        Enis Soztutar
      3. 10616-add.txt
        0.8 kB
        Devaraj Das
      4. 10616-4.1.txt
        18 kB
        Devaraj Das
      5. 10616-4.txt
        18 kB
        Devaraj Das
      6. 10616-3.txt
        17 kB
        Devaraj Das
      7. 10616-2.2.txt
        17 kB
        Devaraj Das
      8. 10616-1.txt
        13 kB
        Devaraj Das

        Activity

        Hide
        Devaraj Das added a comment -

        First cut. On top of the hbase-10572_v0.patch from HBASE-10572.

        Show
        Devaraj Das added a comment - First cut. On top of the hbase-10572_v0.patch from HBASE-10572 .
        Hide
        Nick Dimiduk added a comment -
        //TODO: should we record each GET in the batch as one READ
        

        This should be necessary if you want to directly compare different runs (ie, no batching, batchsize=10, batchsize=50, &c.), so yes, I think it's desirable. OTOH, I don't have this in PerfEval yet, though I believe the reported throughput metrics are correct for this use.

        +          // A sanity check for the key range.
        +          if (k < startKey || k >= endKey) {
        +            numReadErrors.incrementAndGet();
        +            throw new AssertionError("Load tester logic error: proposed key " +
        +                "to read " + k + " is out of range (startKey=" + startKey +
        +                ", endKey=" + endKey + ")");
        +          }
        

        This appears to throw when a key falls outside of the target row range. Above that, the keys[] is populated up to target batchsize (do {} while (isMultiGet && numIter < batchSize);). Does that mean this assertion fails in the event that the batch boundary and key range are not precisely aligned? I'd expect this to happen at the end of every range unless you have other calculations to enforce this alignment. PerfEval handles this case by checking for any remaining buffered items and sending that partial batch at test conclusion.

        Show
        Nick Dimiduk added a comment - //TODO: should we record each GET in the batch as one READ This should be necessary if you want to directly compare different runs (ie, no batching, batchsize=10, batchsize=50, &c.), so yes, I think it's desirable. OTOH, I don't have this in PerfEval yet, though I believe the reported throughput metrics are correct for this use. + // A sanity check for the key range. + if (k < startKey || k >= endKey) { + numReadErrors.incrementAndGet(); + throw new AssertionError("Load tester logic error: proposed key " + + "to read " + k + " is out of range (startKey=" + startKey + + ", endKey=" + endKey + ")"); + } This appears to throw when a key falls outside of the target row range. Above that, the keys[] is populated up to target batchsize ( do {} while (isMultiGet && numIter < batchSize); ). Does that mean this assertion fails in the event that the batch boundary and key range are not precisely aligned? I'd expect this to happen at the end of every range unless you have other calculations to enforce this alignment. PerfEval handles this case by checking for any remaining buffered items and sending that partial batch at test conclusion.
        Hide
        Nick Dimiduk added a comment -

        looking back at getNextKeyToRead, i see it's generating random keys within the bounds, not a sequence of keys within the bounds. Please disregard my comment about batchsize alignment

        Show
        Nick Dimiduk added a comment - looking back at getNextKeyToRead, i see it's generating random keys within the bounds, not a sequence of keys within the bounds. Please disregard my comment about batchsize alignment
        Hide
        Devaraj Das added a comment -

        Thanks, Nick Dimiduk for the review. Attaching the current WIP patch. The test gets stuck. Debugging.

        Show
        Devaraj Das added a comment - Thanks, Nick Dimiduk for the review. Attaching the current WIP patch. The test gets stuck. Debugging.
        Hide
        Nick Dimiduk added a comment -

        Is this test still getting wedged? Do you have a new patch?

        Show
        Nick Dimiduk added a comment - Is this test still getting wedged? Do you have a new patch?
        Hide
        Devaraj Das added a comment -

        There is no change in the test code since the last patch.
        The test used to get stuck because of legit issues (tracked in HBASE-10634). The new runs don't get stuck but they fail due to "READ FAILURES"

        2014-03-10 00:56:06,068 INFO  [Thread-55] util.MultiThreadedAction: [R:40] Keys=6829000, cols=82.0 M, time=00:19:36 Overall: [keys/s= 5803, latency=5 ms] Current: [keys/s=6000, latency=5 ms], verified=6827969, READ FAILURES=45, get_timeouts=45
        
        Show
        Devaraj Das added a comment - There is no change in the test code since the last patch. The test used to get stuck because of legit issues (tracked in HBASE-10634 ). The new runs don't get stuck but they fail due to "READ FAILURES" 2014-03-10 00:56:06,068 INFO [Thread-55] util.MultiThreadedAction: [R:40] Keys=6829000, cols=82.0 M, time=00:19:36 Overall: [keys/s= 5803, latency=5 ms] Current: [keys/s=6000, latency=5 ms], verified=6827969, READ FAILURES=45, get_timeouts=45
        Hide
        Enis Soztutar added a comment -

        HBASE-10572 is resolved. Let's get this in as well.

        LoadTestTool args have convention for using underscore, instead of camel case:

        +  public static final String OPT_MULTIGET = "multigetBatchSize";
        

        Need Javadoc on the new IT test class.
        From my reading of the code, the batch size will not be filled for almost all the time, because we will be skipping keys, and keysForThisReader will not be filled at all.

        +          if (k % numThreads != readerId ||
        +              writer != null && writer.failedToWriteKey(k)) {
        +            // Skip keys that this thread should not read, as well as the keys
        +            // that we know the writer failed to write.
        +            continue;
        +          }
        

        There is some code duplication. Can we simplify some paths, where a Get[] with length 1 is a simple get, and multi get if length > 1.

        Show
        Enis Soztutar added a comment - HBASE-10572 is resolved. Let's get this in as well. LoadTestTool args have convention for using underscore, instead of camel case: + public static final String OPT_MULTIGET = "multigetBatchSize" ; Need Javadoc on the new IT test class. From my reading of the code, the batch size will not be filled for almost all the time, because we will be skipping keys, and keysForThisReader will not be filled at all. + if (k % numThreads != readerId || + writer != null && writer.failedToWriteKey(k)) { + // Skip keys that this thread should not read, as well as the keys + // that we know the writer failed to write. + continue ; + } There is some code duplication. Can we simplify some paths, where a Get[] with length 1 is a simple get, and multi get if length > 1.
        Hide
        Devaraj Das added a comment -

        This patch should address your comments. BTW on the keysForThisReader not getting enough keys, I don't think it's an issue. Assume you have 40 threads and some keys, the first reader will get

        {0,40,80....}

        , and the second reader will get

        {1,41,...}

        and so on. So all keys are covered. Am i missing something?

        Show
        Devaraj Das added a comment - This patch should address your comments. BTW on the keysForThisReader not getting enough keys, I don't think it's an issue. Assume you have 40 threads and some keys, the first reader will get {0,40,80....} , and the second reader will get {1,41,...} and so on. So all keys are covered. Am i missing something?
        Hide
        Devaraj Das added a comment -

        This is a better patch.

        Show
        Devaraj Das added a comment - This is a better patch.
        Hide
        Enis Soztutar added a comment -

        Thanks Devaraj. Patch looks good. +1 if you have tested it out.

        Show
        Enis Soztutar added a comment - Thanks Devaraj. Patch looks good. +1 if you have tested it out.
        Hide
        Devaraj Das added a comment -

        Thanks Enis for the review. This patch has a minor update to a log message. The test passes for smaller values of batchsize but fails for larger values. But I think this can be committed and the failure investigated as a follow on (the failure is very likely because of a bug in the AsyncProcess code as opposed to a test issue).

        Nick Dimiduk, any comments before I commit?

        Show
        Devaraj Das added a comment - Thanks Enis for the review. This patch has a minor update to a log message. The test passes for smaller values of batchsize but fails for larger values. But I think this can be committed and the failure investigated as a follow on (the failure is very likely because of a bug in the AsyncProcess code as opposed to a test issue). Nick Dimiduk , any comments before I commit?
        Hide
        Nick Dimiduk added a comment -

        v4.1 looks good to me. +1

        Show
        Nick Dimiduk added a comment - v4.1 looks good to me. +1
        Hide
        Devaraj Das added a comment -

        Committed. Thanks for the reviews.

        Show
        Devaraj Das added a comment - Committed. Thanks for the reviews.
        Hide
        Devaraj Das added a comment -

        I had to do the bugfix as in the attached patch to handle the case of multiget_batchsize == 0.

        Show
        Devaraj Das added a comment - I had to do the bugfix as in the attached patch to handle the case of multiget_batchsize == 0.
        Hide
        Ted Yu added a comment -

        +1 on addendum.

        Show
        Ted Yu added a comment - +1 on addendum.
        Hide
        Enis Soztutar added a comment -

        Attaching rebased patch for master that is committed

        Show
        Enis Soztutar added a comment - Attaching rebased patch for master that is committed
        Hide
        Enis Soztutar added a comment -

        Attaching rebased patch for master that is committed

        Show
        Enis Soztutar added a comment - Attaching rebased patch for master that is committed
        Hide
        Enis Soztutar added a comment -

        Committed to master as part of hbase-10070 branch merge

        Show
        Enis Soztutar added a comment - Committed to master as part of hbase-10070 branch merge
        Hide
        Enis Soztutar added a comment -

        Committed to master as part of hbase-10070 branch merge

        Show
        Enis Soztutar added a comment - Committed to master as part of hbase-10070 branch merge
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK #5245 (See https://builds.apache.org/job/HBase-TRUNK/5245/)
        HBASE-10616. Integration test for multi-get calls (enis: rev 41116848e5a1f9ceb3558ce654f5aff2064e2158)

        • hbase-it/src/test/java/org/apache/hadoop/hbase/test/IntegrationTestTimeBoundedMultiGetRequestsWithRegionReplicas.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/util/LoadTestTool.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReaderWithACL.java
        • hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java
        • hbase-it/src/test/java/org/apache/hadoop/hbase/test/IntegrationTestTimeBoundedRequestsWithRegionReplicas.java
          HBASE-10616. Addendum that fixes the case of multiget_batchsize == 1 (enis: rev ffabf9ba9b70baa679cca6cf804ef4ec275fdccc)
        • hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK #5245 (See https://builds.apache.org/job/HBase-TRUNK/5245/ ) HBASE-10616 . Integration test for multi-get calls (enis: rev 41116848e5a1f9ceb3558ce654f5aff2064e2158) hbase-it/src/test/java/org/apache/hadoop/hbase/test/IntegrationTestTimeBoundedMultiGetRequestsWithRegionReplicas.java hbase-server/src/test/java/org/apache/hadoop/hbase/util/LoadTestTool.java hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReaderWithACL.java hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java hbase-it/src/test/java/org/apache/hadoop/hbase/test/IntegrationTestTimeBoundedRequestsWithRegionReplicas.java HBASE-10616 . Addendum that fixes the case of multiget_batchsize == 1 (enis: rev ffabf9ba9b70baa679cca6cf804ef4ec275fdccc) hbase-server/src/test/java/org/apache/hadoop/hbase/util/MultiThreadedReader.java
        Hide
        Enis Soztutar added a comment -

        Closing this issue after 0.99.0 release.

        Show
        Enis Soztutar added a comment - Closing this issue after 0.99.0 release.

          People

          • Assignee:
            Devaraj Das
            Reporter:
            Devaraj Das
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development