Details

    • Type: New Feature New Feature
    • Status: Patch Available
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      The contract of ClientScanner is to return rows in sort order. That limits the order in which region can be scanned.
      I propose a simple ParallelScanner that does not have this requirement and queries regions in parallel, return whatever gets returned first.

      This is generally useful for scans that filter a lot of data on the server, or in cases where the client can very quickly react to the returned data.

      I have a simple prototype (doesn't do error handling right, and might be a bit heavy on the synchronization side - it used a BlockingQueue to hand data between the client using the scanner and the threads doing the scanning, it also could potentially starve some scanners long enugh to time out at the server).
      On the plus side, it's only a 130 lines of code.

      1. ParallelClientScanner.java
        4 kB
        Lars Hofhansl
      2. ParallelClientScanner.java
        7 kB
        Lars Hofhansl
      3. 9272-0.94.txt
        16 kB
        Lars Hofhansl
      4. 9272-0.94-v2.txt
        20 kB
        Lars Hofhansl
      5. 9272-0.94-v3.txt
        18 kB
        Lars Hofhansl
      6. 9272-0.94-v4.txt
        17 kB
        Lars Hofhansl
      7. 9272-trunk.txt
        17 kB
        Lars Hofhansl
      8. 9272-trunk-v2.txt
        18 kB
        Lars Hofhansl
      9. 9272-trunk-v3.txt
        17 kB
        Lars Hofhansl
      10. 9272-trunk-v3.txt
        17 kB
        stack
      11. 9272-trunk-v4.txt
        17 kB
        Lars Hofhansl

        Activity

        Hide
        Jean-Marc Spaggiari added a comment -

        I like the idea.

        At the end, that will looks like a MR where all the regions are scanned in //, right?

        Show
        Jean-Marc Spaggiari added a comment - I like the idea. At the end, that will looks like a MR where all the regions are scanned in //, right?
        Hide
        Lars Hofhansl added a comment -

        More like a MultiGet that farms requests out to multiple RegionServers at the same time, although I am using a different threading model (fixed number of threads and an unbounded waiting queue, rather than the reverse).
        There are a lot of options. Right now each region becomes a task and is scheduled on a threadpool. Could also group by regionserver.
        Obviously this only makes sense when the scan will touch a "reasonable" number of a regions.

        Show
        Lars Hofhansl added a comment - More like a MultiGet that farms requests out to multiple RegionServers at the same time, although I am using a different threading model (fixed number of threads and an unbounded waiting queue, rather than the reverse). There are a lot of options. Right now each region becomes a task and is scheduled on a threadpool. Could also group by regionserver. Obviously this only makes sense when the scan will touch a "reasonable" number of a regions.
        Hide
        Lars Hofhansl added a comment -

        UNFINISHED, UNTESTED. Just want to park it somewhere.

        Show
        Lars Hofhansl added a comment - UNFINISHED, UNTESTED. Just want to park it somewhere.
        Hide
        Anoop Sam John added a comment -

        Same as HBASE-1935 ? +1 on having some thing like this..

        Show
        Anoop Sam John added a comment - Same as HBASE-1935 ? +1 on having some thing like this..
        Hide
        Lars Hofhansl added a comment -

        Yeah, that was the one where I convinced Stack that we do not need this.

        Show
        Lars Hofhansl added a comment - Yeah, that was the one where I convinced Stack that we do not need this.
        Hide
        Lars Hofhansl added a comment -

        Compared to HBASE-1935 this is much simpler since it still uses ClientScanner under the hood (which I had since factored in its own class).
        Using ClientScanner also has the benefit that this is resistant to concurrent splits.

        Simple perf test with 30m rows, 1 col, 100 byte values. Split into 16 regions on a cluster with 16 region servers.
        The performance speedup (scan latency here) is proportional to the number of threads used when most data is filtered at the server (as expected) - of course the cluster was not otherwise busy.
        Even when all rows are returned I see following (scanner caching: 100), buffer size in ParallelScanner was 1000 and 10000 (made no difference perf wise):

        • Running the ParallelScanner with 50 threads: 40.8s.
        • Running the ParallelScanner with 20 threads: 40.3s.
        • Running the ParallelScanner with 16 threads: 40.3s.
        • Running the ParallelScanner with 10 threads: 59.6s.
        • Running the ParallelScanner with 1 thread: 316
        • Running the standard scanner: 309

        So there is a 1.5% synchronization/context-switching overhead.
        It looks like this general approach a viable and by using ClientScanner it is also ridiculously simple.

        Show
        Lars Hofhansl added a comment - Compared to HBASE-1935 this is much simpler since it still uses ClientScanner under the hood (which I had since factored in its own class). Using ClientScanner also has the benefit that this is resistant to concurrent splits. Simple perf test with 30m rows, 1 col, 100 byte values. Split into 16 regions on a cluster with 16 region servers. The performance speedup (scan latency here) is proportional to the number of threads used when most data is filtered at the server (as expected) - of course the cluster was not otherwise busy. Even when all rows are returned I see following (scanner caching: 100), buffer size in ParallelScanner was 1000 and 10000 (made no difference perf wise): Running the ParallelScanner with 50 threads: 40.8s. Running the ParallelScanner with 20 threads: 40.3s. Running the ParallelScanner with 16 threads: 40.3s. Running the ParallelScanner with 10 threads: 59.6s. Running the ParallelScanner with 1 thread: 316 Running the standard scanner: 309 So there is a 1.5% synchronization/context-switching overhead. It looks like this general approach a viable and by using ClientScanner it is also ridiculously simple.
        Hide
        Jean-Marc Spaggiari added a comment -

        Is there a way to make that optional so if people want to not have the 1.5% overhead they can? Like, when they know they will need all the data or something like that?

        Show
        Jean-Marc Spaggiari added a comment - Is there a way to make that optional so if people want to not have the 1.5% overhead they can? Like, when they know they will need all the data or something like that?
        Hide
        Lars Hofhansl added a comment -

        Oh yeah. You'd have to instantiate the ParallelScanner object yourself in the client anyway.

        Show
        Lars Hofhansl added a comment - Oh yeah. You'd have to instantiate the ParallelScanner object yourself in the client anyway.
        Hide
        Lars Hofhansl added a comment -

        A little cleanup. Deals with exceptions now (retrows only the first one).

        Note: This is client side only, using only public HBase interfaces, so this does not need to be part of HBase.

        Also re: the 1.5% this happens only when you use ParallelScanner with a single thread, should use ClientScanner (what HTable.getScanner returns) in that case anyway.

        Show
        Lars Hofhansl added a comment - A little cleanup. Deals with exceptions now (retrows only the first one). Note: This is client side only, using only public HBase interfaces, so this does not need to be part of HBase. Also re: the 1.5% this happens only when you use ParallelScanner with a single thread, should use ClientScanner (what HTable.getScanner returns) in that case anyway.
        Hide
        Anoop Sam John added a comment -

        Encouraging numbers Lars...
        Can we have a test with much more #regions? The #threads should be well below #regions.. Can the client be little more intelligent so as to distribute the load to all the RSs at a given point in time.. Say 10 threads in client side and 10 RSs and there are 1000 regions. At a given point in time, there is chance that all 10 client threads contacting regions which are in same RS. So all other RSs will be idle for that time. May be for a begining a simple patch would be enough. These are improvements we can try later also. Good one Lars.

        Show
        Anoop Sam John added a comment - Encouraging numbers Lars... Can we have a test with much more #regions? The #threads should be well below #regions.. Can the client be little more intelligent so as to distribute the load to all the RSs at a given point in time.. Say 10 threads in client side and 10 RSs and there are 1000 regions. At a given point in time, there is chance that all 10 client threads contacting regions which are in same RS. So all other RSs will be idle for that time. May be for a begining a simple patch would be enough. These are improvements we can try later also. Good one Lars.
        Hide
        Lars Hofhansl added a comment -

        I was thinking about this too (i.e. keeping all RSs busy). On the other hand I was trying to keep this simple, assuming that in most cases the region to server assignment would be more or less random.
        With some number of threads and a reasonable sized cluster (without which a parallel scanner does not help much anyway), one would assume a fairly nice load distribution.

        So a test with more regions should see the same speedup, there is nothing inherently costly per region (the ClientScanners will need to find the region again, but it should be cached).

        There are other considerations too. For example, instead of having a task per Region, one could split the requested rowkey space into N slices (using the region boundaries as a poor-mans histogram by assuming that all regions will be of roughly the same size in bytes). In that case one would keep the number of threads unlimited but instead limit the number of tasks (i.e. slices).

        (also above the penalty was 2.2% rather than 1.5% - but that was just a single run anyway)

        Will do a test with more regions.

        Show
        Lars Hofhansl added a comment - I was thinking about this too (i.e. keeping all RSs busy). On the other hand I was trying to keep this simple, assuming that in most cases the region to server assignment would be more or less random. With some number of threads and a reasonable sized cluster (without which a parallel scanner does not help much anyway), one would assume a fairly nice load distribution. So a test with more regions should see the same speedup, there is nothing inherently costly per region (the ClientScanners will need to find the region again, but it should be cached). There are other considerations too. For example, instead of having a task per Region, one could split the requested rowkey space into N slices (using the region boundaries as a poor-mans histogram by assuming that all regions will be of roughly the same size in bytes). In that case one would keep the number of threads unlimited but instead limit the number of tasks (i.e. slices). (also above the penalty was 2.2% rather than 1.5% - but that was just a single run anyway) Will do a test with more regions.
        Hide
        Lars Hofhansl added a comment - - edited

        Some more data:
        30m rows, 2 CFs, 5 columns each, with 100 bytes values. Split into 128 regions on 15 region servers.

        When all data is returned - this is limited by what the client can consume (via the network and by actually iterating over the result). All numbers in seconds:

        ClientScanner 1 thread 2 threads 5 threads 10 threads 50 threads
        519 529 303 192 189 187

        When all is filtered with a ValueFilter on the server (as in an analytics query):

        ClientScanner 1 thread 2 threads 5 threads 10 threads 50 threads
        53.3 53.3 28.4 11.6 6.42 1.88
        Show
        Lars Hofhansl added a comment - - edited Some more data: 30m rows, 2 CFs, 5 columns each, with 100 bytes values. Split into 128 regions on 15 region servers. When all data is returned - this is limited by what the client can consume (via the network and by actually iterating over the result). All numbers in seconds: ClientScanner 1 thread 2 threads 5 threads 10 threads 50 threads 519 529 303 192 189 187 When all is filtered with a ValueFilter on the server (as in an analytics query): ClientScanner 1 thread 2 threads 5 threads 10 threads 50 threads 53.3 53.3 28.4 11.6 6.42 1.88
        Hide
        Lars Hofhansl added a comment -

        You might notice that 30m * 10 * 108 = 30gb is actually impossible to pipe over a 1ge link in 187s.
        Turns out I had only scanned one of the column families in my test code. So the numbers are for selecting only 5 of columns.
        Even then we're approaching what can be streamed of a 1ge link. 80mb/s out of the max 125mb/s.

        In the case where everything is filtered we're churning through 15gb in the cache in less then 2s in 100 byte chunks. Not too bad.

        Show
        Lars Hofhansl added a comment - You might notice that 30m * 10 * 108 = 30gb is actually impossible to pipe over a 1ge link in 187s. Turns out I had only scanned one of the column families in my test code. So the numbers are for selecting only 5 of columns. Even then we're approaching what can be streamed of a 1ge link. 80mb/s out of the max 125mb/s. In the case where everything is filtered we're churning through 15gb in the cache in less then 2s in 100 byte chunks. Not too bad.
        Hide
        Lars Hofhansl added a comment -

        So what do folks want to see from this? I have a version now that does RS round robin and also reduces synchronization a bit by passing Exceptions as Result subclasses through the Queue between the scanners and the reader thread.
        I get the fairest scheduling by submitting single region tasks in RS round robin order to a thread pool. The only down side is that the with this kind of scheduling the outbounded pools used for other HTable operations cannot be used here. I can work around that by pulling doing the task queuing myself outside of the Threadpool.

        Lastly a theme similar to this can be efficiently used for a sorted prefetching scanner - one just spawnes off N threads this time in Region order, each writing into their own queues, and then read them in order.

        Show
        Lars Hofhansl added a comment - So what do folks want to see from this? I have a version now that does RS round robin and also reduces synchronization a bit by passing Exceptions as Result subclasses through the Queue between the scanners and the reader thread. I get the fairest scheduling by submitting single region tasks in RS round robin order to a thread pool. The only down side is that the with this kind of scheduling the outbounded pools used for other HTable operations cannot be used here. I can work around that by pulling doing the task queuing myself outside of the Threadpool. Lastly a theme similar to this can be efficiently used for a sorted prefetching scanner - one just spawnes off N threads this time in Region order, each writing into their own queues, and then read them in order.
        Hide
        Lars Hofhansl added a comment -

        So here's a sample patch against 0.94. It does the following:

        1. An API to parallelize a single Scan.
        2. Round robin across RegionServers
        3. Builds its own task queue in order not to rely on a specifically configured thread pool (i.e. the HTable's pool can be used)
        4. explores ways of automated scaling. The parallelism is controlled by a scaling factor that takes the number of a region server touched by the scan into account
        5. An alternate API where the caller can pass in a set of Splits (in form of Scans) and then those are executed on the pool
        6. limits all thread synchronization to the a BlockingQueue, which (in theory) allows the reader and the writer to lock independently
        7. to avoid other synchronization, marker objects are passed to indicate when the thread is done or encountered an exception
        8. Also hooked this up with HTable (which is the only questionable - IMHO - part of this, since it changes HTableInterface and could break client application that directly implement HTableInterface). This part is not strictly needed, ParallelClientScanner can be used on its own.
        9. Pushes a bit more common code into AbstractClientScanner.

        Please let me know what you think. If direction is good I'll add tests and make a trunk patch.

        Show
        Lars Hofhansl added a comment - So here's a sample patch against 0.94. It does the following: An API to parallelize a single Scan. Round robin across RegionServers Builds its own task queue in order not to rely on a specifically configured thread pool (i.e. the HTable's pool can be used) explores ways of automated scaling. The parallelism is controlled by a scaling factor that takes the number of a region server touched by the scan into account An alternate API where the caller can pass in a set of Splits (in form of Scans) and then those are executed on the pool limits all thread synchronization to the a BlockingQueue, which (in theory) allows the reader and the writer to lock independently to avoid other synchronization, marker objects are passed to indicate when the thread is done or encountered an exception Also hooked this up with HTable (which is the only questionable - IMHO - part of this, since it changes HTableInterface and could break client application that directly implement HTableInterface). This part is not strictly needed, ParallelClientScanner can be used on its own. Pushes a bit more common code into AbstractClientScanner. Please let me know what you think. If direction is good I'll add tests and make a trunk patch.
        Hide
        Ted Yu added a comment - - edited
        +      for (Iterator<Map.Entry<HRegionLocation, Queue<Scan>>> it = tasks.entrySet().iterator(); it.hasNext();) {
        +        Scan next = it.next().getValue().poll();
        +        if (next == null) {
        +          it.remove();
        

        If there is more than 1 Scan for a Queue (for some HRegionLocation), would the second and subsequent Scan's be skipped due to call to it.next() above ?

        Show
        Ted Yu added a comment - - edited + for (Iterator<Map.Entry<HRegionLocation, Queue<Scan>>> it = tasks.entrySet().iterator(); it.hasNext();) { + Scan next = it.next().getValue().poll(); + if (next == null ) { + it.remove(); If there is more than 1 Scan for a Queue (for some HRegionLocation), would the second and subsequent Scan's be skipped due to call to it.next() above ?
        Hide
        Lars Hofhansl added a comment -

        It shouldn't. it.next() places the marker; if that turns to be empty it is removed. The next iteration than forwards the marker. The Javadoc says: "Removes from the underlying collection the last element returned by the iterator (optional operation)."

        Show
        Lars Hofhansl added a comment - It shouldn't. it.next() places the marker; if that turns to be empty it is removed. The next iteration than forwards the marker. The Javadoc says: "Removes from the underlying collection the last element returned by the iterator (optional operation)."
        Hide
        Ted Yu added a comment -
        +   * close the parallel scanner, callers are strongly encouraged to call this method
        +   * doesn't wait until the threapool actually closes
        +   */
        +  @Override
        +  public void close() {
        +  }
        

        Should ExecutorService pool be shut down in the above method ?

        +      } catch (InterruptedException ix) {
        +        // ignore
        

        Please restore interrupt status.

        Patch for trunk would be nice.

        Show
        Ted Yu added a comment - + * close the parallel scanner, callers are strongly encouraged to call this method + * doesn't wait until the threapool actually closes + */ + @Override + public void close() { + } Should ExecutorService pool be shut down in the above method ? + } catch (InterruptedException ix) { + // ignore Please restore interrupt status. Patch for trunk would be nice.
        Hide
        Lars Hofhansl added a comment -

        Fixes minor issue, enables shell support in scan and count.

        Show
        Lars Hofhansl added a comment - Fixes minor issue, enables shell support in scan and count.
        Hide
        Lars Hofhansl added a comment -

        Thanks Ted.
        The thread pool is always passes in, so ParallelClientScanner does not manage that in any event. Yeah, interrupted status should be restored.
        Also need a way to kill the other threads running when one threads throws an exception. Previously I was using an exclusive thread pool, but now it is shared.

        Will make a trunk patch too.

        Show
        Lars Hofhansl added a comment - Thanks Ted. The thread pool is always passes in, so ParallelClientScanner does not manage that in any event. Yeah, interrupted status should be restored. Also need a way to kill the other threads running when one threads throws an exception. Previously I was using an exclusive thread pool, but now it is shared. Will make a trunk patch too.
        Hide
        Lars Hofhansl added a comment -

        Patch with fewer API changes.
        Still need to do exception handling.
        I'll probably go back to the earlier version of having a dedicated threadpool for a parallel scanner, for three reasons:

        1. It's easy to kill all outstanding tasks on dedicated pool
        2. This is for long running scans anyway, so the cost of a dedicated pool is amortized nicely.
        3. The scanning does not interfere with the other (more latency sensitive - such as MultiGet) operations.

        Will also make a trunk patch soon. Promised.

        Show
        Lars Hofhansl added a comment - Patch with fewer API changes. Still need to do exception handling. I'll probably go back to the earlier version of having a dedicated threadpool for a parallel scanner, for three reasons: It's easy to kill all outstanding tasks on dedicated pool This is for long running scans anyway, so the cost of a dedicated pool is amortized nicely. The scanning does not interfere with the other (more latency sensitive - such as MultiGet) operations. Will also make a trunk patch soon. Promised.
        Hide
        Lars Hofhansl added a comment -

        Parking another version. Back to a dedicated threadpool and handling thread interruption correctly.

        Show
        Lars Hofhansl added a comment - Parking another version. Back to a dedicated threadpool and handling thread interruption correctly.
        Hide
        Lars Hofhansl added a comment -

        Here finally is a trunk version.

        Show
        Lars Hofhansl added a comment - Here finally is a trunk version.
        Hide
        Ted Yu added a comment -

        License is missing for ParallelClientScanner.java
        Please add annotation for audience.

        +  // reader interface
        +  public static interface ResultReader {
        ...
        +  // writer interface
        +  public static interface ResultWriter {
        

        Looks like the above classes can be private.

        +      } catch (InterruptedException ix) {
        +        // ignore
        

        Restore interrupt status ?

        Show
        Ted Yu added a comment - License is missing for ParallelClientScanner.java Please add annotation for audience. + // reader interface + public static interface ResultReader { ... + // writer interface + public static interface ResultWriter { Looks like the above classes can be private. + } catch (InterruptedException ix) { + // ignore Restore interrupt status ?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12606939/9272-trunk.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        -1 javadoc. The javadoc tool appears to have generated 3 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        -1 release audit. The applied patch generated 1 release audit warnings (more than the trunk's current 0 warnings).

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//testReport/
        Release audit warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12606939/9272-trunk.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 3 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. -1 release audit . The applied patch generated 1 release audit warnings (more than the trunk's current 0 warnings). +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//testReport/ Release audit warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7475//console This message is automatically generated.
        Hide
        Lars Hofhansl added a comment -

        Addressing Ted's comments.

        Show
        Lars Hofhansl added a comment - Addressing Ted's comments.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12607251/9272-trunk-v2.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        -1 javadoc. The javadoc tool appears to have generated 3 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12607251/9272-trunk-v2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 3 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 findbugs . The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7485//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        +1, after javadoc warnings are fixed.

        Show
        Ted Yu added a comment - +1, after javadoc warnings are fixed.
        Hide
        Lars Hofhansl added a comment -

        Thanks Ted. Will fix the Javadoc warnings.
        The next question is: We can check this in integrated with HTable as is, or we could commit this as example code. I'd be fine either way.

        Show
        Lars Hofhansl added a comment - Thanks Ted. Will fix the Javadoc warnings. The next question is: We can check this in integrated with HTable as is, or we could commit this as example code. I'd be fine either way.
        Hide
        Lars Hofhansl added a comment -

        stack, wanna have a look?

        Show
        Lars Hofhansl added a comment - stack , wanna have a look?
        Hide
        stack added a comment -

        Looks great. +1

        Minor stuff to address on commit.

        nit: try/finally for this bit

        + List<HRegionLocation> locs = t.getRegionsInRange(scan.getStartRow(), scan.getStopRow());
        + t.close();

        nit: presize + Map<HRegionLocation, Queue<Scan>> tasks = new HashMap<HRegionLocation, Queue<Scan>>(); – you know how many regions

        nit: + * doesn't wait until the threapool actually closes – spelling

        You mean volatile here? + private transient double parallelScaling = 0;

        Agree it should be in HTable. I like addtion to the shell.

        'Task' is too generic a name for the class and the ReaderWriter looks like a Queue but as long as these classes are kept private should be fine.

        MyResult is a bit hokey for a class name. Done or DoneMarker? Bit more doc on its operation (it makes sense when you take a look but not reading the class alone).

        Show
        stack added a comment - Looks great. +1 Minor stuff to address on commit. nit: try/finally for this bit + List<HRegionLocation> locs = t.getRegionsInRange(scan.getStartRow(), scan.getStopRow()); + t.close(); nit: presize + Map<HRegionLocation, Queue<Scan>> tasks = new HashMap<HRegionLocation, Queue<Scan>>(); – you know how many regions nit: + * doesn't wait until the threapool actually closes – spelling You mean volatile here? + private transient double parallelScaling = 0; Agree it should be in HTable. I like addtion to the shell. 'Task' is too generic a name for the class and the ReaderWriter looks like a Queue but as long as these classes are kept private should be fine. MyResult is a bit hokey for a class name. Done or DoneMarker? Bit more doc on its operation (it makes sense when you take a look but not reading the class alone).
        Hide
        Lars Hofhansl added a comment -

        Cool. Thanks Stack.

        You mean volatile here? + private transient double parallelScaling = 0;

        I did mean transient (just to indicate that this is not serialized with the Scan object, since it is only in the client code).

        Fill fix up the rest and post a new patch tomorrow.

        Show
        Lars Hofhansl added a comment - Cool. Thanks Stack. You mean volatile here? + private transient double parallelScaling = 0; I did mean transient (just to indicate that this is not serialized with the Scan object, since it is only in the client code). Fill fix up the rest and post a new patch tomorrow.
        Hide
        Lars Hofhansl added a comment -

        Updated trunk version:

        • renamed some inner classes
        • fixed javadocs
        • addressed Stack's comments
        Show
        Lars Hofhansl added a comment - Updated trunk version: renamed some inner classes fixed javadocs addressed Stack's comments
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12607740/9272-trunk-v3.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.coprocessor.TestRegionObserverScannerOpenHook

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7512//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7512//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12607740/9272-trunk-v3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. +1 javadoc . The javadoc tool did not generate any warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.coprocessor.TestRegionObserverScannerOpenHook Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7512//testReport/ Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7512//console This message is automatically generated.
        Hide
        stack added a comment -

        +1 on v3

        On commit add a bit of doc around the 'scaling factor' in javadoc – you say what it is in the shell but in the methods so its confusing seeing a double returned.

        Is the test failure yours? Let me rerun the patch.

        Show
        stack added a comment - +1 on v3 On commit add a bit of doc around the 'scaling factor' in javadoc – you say what it is in the shell but in the methods so its confusing seeing a double returned. Is the test failure yours? Let me rerun the patch.
        Hide
        stack added a comment -

        Retrying hadoopqa.

        If you are going to put this in 0.94, I suppose it has to go into 0.96.

        Show
        stack added a comment - Retrying hadoopqa. If you are going to put this in 0.94, I suppose it has to go into 0.96.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12609896/9272-trunk-v3.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7607//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12609896/9272-trunk-v3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7607//console This message is automatically generated.
        Hide
        stack added a comment -

        It needs a rebase?

        Show
        stack added a comment - It needs a rebase?
        Hide
        Lars Hofhansl added a comment -

        Lemme look into this. I have been quite busy the past few days.

        Show
        Lars Hofhansl added a comment - Lemme look into this. I have been quite busy the past few days.
        Hide
        Lars Hofhansl added a comment -

        TestRegionObserverScannerOpenHook

        I've seen this in other test runs, so this is probably not related.

        Show
        Lars Hofhansl added a comment - TestRegionObserverScannerOpenHook I've seen this in other test runs, so this is probably not related.
        Hide
        Lars Hofhansl added a comment -

        Rebased. TestRegionObserverScannerOpenHook passes locally for me.

        Should this support scan metrics? It would need to aggregate the individual scan metrics, and hence need somewhat of a redesign (need to keep track of the all the individual tasks via futures and then wait for them to finish).
        Or could maybe pass along with the marker result at the end.

        Show
        Lars Hofhansl added a comment - Rebased. TestRegionObserverScannerOpenHook passes locally for me. Should this support scan metrics? It would need to aggregate the individual scan metrics, and hence need somewhat of a redesign (need to keep track of the all the individual tasks via futures and then wait for them to finish). Or could maybe pass along with the marker result at the end.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12610225/9272-trunk-v4.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

        -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12610225/9272-trunk-v4.txt against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 1 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/7629//console This message is automatically generated.
        Hide
        Lars Hofhansl added a comment -

        The metric stuff gets obtuse pretty quickly, especially when the parallel scanner is closed and all the threads are interrupted/canceled, before they can report their metrics.

        This also needs some tests. I'm moving this 0.94.14.

        Show
        Lars Hofhansl added a comment - The metric stuff gets obtuse pretty quickly, especially when the parallel scanner is closed and all the threads are interrupted/canceled, before they can report their metrics. This also needs some tests. I'm moving this 0.94.14.
        Hide
        Enis Soztutar added a comment -

        The metric stuff gets obtuse pretty quickly, especially when the parallel scanner is closed and all the threads are interrupted/canceled

        Slightly related, I think pushing the metrics via Scan is broken. The scan should be a pojo, and the metrics should be obtained from the Scanner interface. I am doing a similar thing in HBASE-8369 where you can obtain the metrics via scanner, not the scan.

        Show
        Enis Soztutar added a comment - The metric stuff gets obtuse pretty quickly, especially when the parallel scanner is closed and all the threads are interrupted/canceled Slightly related, I think pushing the metrics via Scan is broken. The scan should be a pojo, and the metrics should be obtained from the Scanner interface. I am doing a similar thing in HBASE-8369 where you can obtain the metrics via scanner, not the scan.
        Hide
        Lars Hofhansl added a comment -

        I'm not happy with this, yet.
        Unscheduling for now.

        Show
        Lars Hofhansl added a comment - I'm not happy with this, yet. Unscheduling for now.
        Hide
        Jean-Marc Spaggiari added a comment -

        Did you come up with a version you are happy with?

        Show
        Jean-Marc Spaggiari added a comment - Did you come up with a version you are happy with?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12610225/9272-trunk-v4.txt
        against trunk revision .
        ATTACHMENT ID: 12610225

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        -1 patch. The patch command could not apply the patch.

        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9827//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12610225/9272-trunk-v4.txt against trunk revision . ATTACHMENT ID: 12610225 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/9827//console This message is automatically generated.

          People

          • Assignee:
            Lars Hofhansl
            Reporter:
            Lars Hofhansl
          • Votes:
            0 Vote for this issue
            Watchers:
            19 Start watching this issue

            Dates

            • Created:
              Updated:

              Development