Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-13090

Progress heartbeats for long running scanners

Details

    • New Feature
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 1.1.0, 2.0.0
    • None
    • None
    • Reviewed
    • Hide
      Previously, there was no way to enforce a time limit on scan RPC requests. The server would receive a scan RPC request and take as much time as it needed to accumulate enough results to reach a limit or exhaust the region. The problem with this approach was that, in the case of a very selective scan, the processing of the scan could take too long and cause timeouts client side.

      With this fix, the server will now enforce a time limit on the execution of scan RPC requests. When a scan RPC request arrives to the server, a time limit is calculated to be half of whichever timeout value is more restictive between the configurations ("hbase.client.scanner.timeout.period" and "hbase.rpc.timeout"). When the time limit is reached, the server will return whatever results it has accumulated up to that point. The results may be empty.

      To ensure that timeout checks do not occur too often (which would hurt the performance of scans), the configuration "hbase.cells.scanned.per.heartbeat.check" has been introduced. This configuration controls how often System.currentTimeMillis() is called to update the progress towards the time limit. Currently, the default value of this configuration value is 10000. Specifying a smaller value will provide a tighter bound on the time limit, but may hurt scan performance due to the higher frequency of calls to System.currentTimeMillis().

      Protobuf models for ScanRequest and ScanResponse have been updated so that heartbeat support can be communicated. Support for heartbeat messages is specified in the request sent to the server via ScanRequest.Builder#setClientHandlesHeartbeats. Only when the server sees that ScanRequest#getClientHandlesHeartbeats() is true will it send heartbeat messages back to the client. A response is marked as a heartbeat message via the boolean flag ScanResponse#getHeartbeatMessage
      Show
      Previously, there was no way to enforce a time limit on scan RPC requests. The server would receive a scan RPC request and take as much time as it needed to accumulate enough results to reach a limit or exhaust the region. The problem with this approach was that, in the case of a very selective scan, the processing of the scan could take too long and cause timeouts client side. With this fix, the server will now enforce a time limit on the execution of scan RPC requests. When a scan RPC request arrives to the server, a time limit is calculated to be half of whichever timeout value is more restictive between the configurations ("hbase.client.scanner.timeout.period" and "hbase.rpc.timeout"). When the time limit is reached, the server will return whatever results it has accumulated up to that point. The results may be empty. To ensure that timeout checks do not occur too often (which would hurt the performance of scans), the configuration "hbase.cells.scanned.per.heartbeat.check" has been introduced. This configuration controls how often System.currentTimeMillis() is called to update the progress towards the time limit. Currently, the default value of this configuration value is 10000. Specifying a smaller value will provide a tighter bound on the time limit, but may hurt scan performance due to the higher frequency of calls to System.currentTimeMillis(). Protobuf models for ScanRequest and ScanResponse have been updated so that heartbeat support can be communicated. Support for heartbeat messages is specified in the request sent to the server via ScanRequest.Builder#setClientHandlesHeartbeats. Only when the server sees that ScanRequest#getClientHandlesHeartbeats() is true will it send heartbeat messages back to the client. A response is marked as a heartbeat message via the boolean flag ScanResponse#getHeartbeatMessage

    Description

      It can be necessary to set very long timeouts for clients that issue scans over large regions when all data in the region might be filtered out depending on scan criteria. This is a usability concern because it can be hard to identify what worst case timeout to use until scans are occasionally/intermittently failing in production, depending on variable scan criteria. It would be better if the client-server scan protocol can send back periodic progress heartbeats to clients as long as server scanners are alive and making progress.

      This is related but orthogonal to streaming scan (HBASE-13071).

      Attachments

        1. HBASE-13090-v7.patch
          97 kB
          Jonathan Lawlor
        2. HBASE-13090-v6.patch
          126 kB
          Jonathan Lawlor
        3. HBASE-13090-v4.patch
          126 kB
          Jonathan Lawlor
        4. HBASE-13090-v3.patch
          129 kB
          Jonathan Lawlor
        5. HBASE-13090-v3.patch
          129 kB
          Jonathan Lawlor
        6. HBASE-13090-v2.patch
          129 kB
          Jonathan Lawlor
        7. HBASE-13090-v1.patch
          98 kB
          Jonathan Lawlor
        8. 13090-branch-1.addendum
          0.7 kB
          Ted Yu

        Issue Links

          Activity

            Perhaps as simple as checking from a timer if any Results have been sent over the preceding interval, forcing back an empty one if none have been sent and no new results are available yet.

            apurtell Andrew Kyle Purtell added a comment - Perhaps as simple as checking from a timer if any Results have been sent over the preceding interval, forcing back an empty one if none have been sent and no new results are available yet.
            larsh Lars Hofhansl added a comment -

            Related to discussion on HBASE-13082.
            There we've been discussing exiting the scan loop to be able to release the lock in order to give flushes/compactions a chance to finish.
            Exiting the scan loop after some time react to slow scans seems a prerequisite for this.

            larsh Lars Hofhansl added a comment - Related to discussion on HBASE-13082 . There we've been discussing exiting the scan loop to be able to release the lock in order to give flushes/compactions a chance to finish. Exiting the scan loop after some time react to slow scans seems a prerequisite for this.
            eshcar Eshcar Hillel added a comment -

            In addition to a timer, or as an alternative to it, one can consider capping prefetched data at the server side by counting the number of rows scanned at each prefetch step. A capping factor limits the max number of rows to be scanned before returning the result to the client.
            This way when the limit is exceeded the server sends whatever data it gathered so far. If no data was found it only sends a heartbeat. When finished scanning the region signal that it is exhausted.

            At the client side, the scanner continuos to scan agains the current region until it is exhausted.

            eshcar Eshcar Hillel added a comment - In addition to a timer, or as an alternative to it, one can consider capping prefetched data at the server side by counting the number of rows scanned at each prefetch step. A capping factor limits the max number of rows to be scanned before returning the result to the client. This way when the limit is exceeded the server sends whatever data it gathered so far. If no data was found it only sends a heartbeat. When finished scanning the region signal that it is exhausted. At the client side, the scanner continuos to scan agains the current region until it is exhausted.

            With HBASE-11544 now in, I was thinking of tackling this one next and was looking for some feedback on the thought process:

            Implementing the timeout server side would involve changes at three different levels:

            • RSRpcServers
            • RegionScannerImpl/ReversedRegionScannerImpl
            • StoreScanner

            The RSRpcServices could maintain a variable; something along the lines of remainingScanTime. This value could be initialized to be some fraction of the scanner timeout (maybe half would be good enough?). On each call to RegionScanner#nextRaw, RSRpcServices would communicate that the RegionScanner can take at most remainingScanTime to retrieve a Result – if a Result cannot be formed in that time, a timeout occurs. The RegionScanner would communicate this same remainingScanTime to the StoreScanner so that calls to InternalScanner#next() may also timeout if they are taking too long.

            Note that if partial Results are NOT supported by the scan configuration (as is the case for small scans, and scans with a filter that requires whole rows to be read before a filtering decision can be made) then the timeout would not be enforceable within StoreScanner but only within RegionScannerImpl and RSRpcServices. This means that it would still be possible to timeout due to a single long running StoreScanner#next() call in the event that partial Results are not supported.

            If a timeout does occur on the server, we would have to decide how this should be communicated back to the Client. I was thinking it would be most appropriate to communicate this back to the client via fields in the ScanResponse rather than flags on the Results in the ScanResponse (there is already a lot of state information implied through the contents of the Results in the ScanResponse and adding more seems like it would complicate things). Something along the lines of a timeoutOccurred boolean flag may be sufficient. Then on the Client side we could decide if enough Results were accumulated prior to the timeout to service the application request or if we must make another RPC to enough Results.

            If anyone else has been thinking about how to approach the solution to this issue or has any other ideas please chime in. Any feedback would be much appreciated.

            jonathan.lawlor Jonathan Lawlor added a comment - With HBASE-11544 now in, I was thinking of tackling this one next and was looking for some feedback on the thought process: Implementing the timeout server side would involve changes at three different levels: RSRpcServers RegionScannerImpl/ReversedRegionScannerImpl StoreScanner The RSRpcServices could maintain a variable; something along the lines of remainingScanTime. This value could be initialized to be some fraction of the scanner timeout (maybe half would be good enough?). On each call to RegionScanner#nextRaw, RSRpcServices would communicate that the RegionScanner can take at most remainingScanTime to retrieve a Result – if a Result cannot be formed in that time, a timeout occurs. The RegionScanner would communicate this same remainingScanTime to the StoreScanner so that calls to InternalScanner#next() may also timeout if they are taking too long. Note that if partial Results are NOT supported by the scan configuration (as is the case for small scans, and scans with a filter that requires whole rows to be read before a filtering decision can be made) then the timeout would not be enforceable within StoreScanner but only within RegionScannerImpl and RSRpcServices. This means that it would still be possible to timeout due to a single long running StoreScanner#next() call in the event that partial Results are not supported. If a timeout does occur on the server, we would have to decide how this should be communicated back to the Client. I was thinking it would be most appropriate to communicate this back to the client via fields in the ScanResponse rather than flags on the Results in the ScanResponse (there is already a lot of state information implied through the contents of the Results in the ScanResponse and adding more seems like it would complicate things). Something along the lines of a timeoutOccurred boolean flag may be sufficient. Then on the Client side we could decide if enough Results were accumulated prior to the timeout to service the application request or if we must make another RPC to enough Results. If anyone else has been thinking about how to approach the solution to this issue or has any other ideas please chime in. Any feedback would be much appreciated.
            stack Michael Stack added a comment -

            Thanks jonathan.lawlor

            Why does RsRpcServices have to be involved? Could remaining scan time not be up in RegionScanner?

            This means that it would still be possible to timeout due to a single long running StoreScanner#next() call in the event that partial Results are not supported.

            Dang. Can we flag these timeouts as "Its your own fault" or, "don't use filter" or "don't short scan" ?

            If you can do the heartbeat usiing ScanResponse rather than pollute Result, that'd be better.

            Looks good to me jonathan.lawlor

            lhofhansl Any input here honey?

            stack Michael Stack added a comment - Thanks jonathan.lawlor Why does RsRpcServices have to be involved? Could remaining scan time not be up in RegionScanner? This means that it would still be possible to timeout due to a single long running StoreScanner#next() call in the event that partial Results are not supported. Dang. Can we flag these timeouts as "Its your own fault" or, "don't use filter" or "don't short scan" ? If you can do the heartbeat usiing ScanResponse rather than pollute Result, that'd be better. Looks good to me jonathan.lawlor lhofhansl Any input here honey?
            stack Michael Stack added a comment - Or mighty andrew.purtell@gmail.com ?

            Why does RsRpcServices have to be involved? Could remaining scan time not be up in RegionScanner?

            I think RsRpcServices needs to be involved because it has the global view of when the scan started. A particular call to RegionScanner#nextRaw may not necessarily cause a timeout, but multiple calls to RegionScanner#nextRaw must be made in order to form the ScanResponse. In other words, a timeout may not be caused by a single call to RegionScanner#nextRaw but rather the accumulated time of all calls necessary to form the ScanResponse.

            jonathan.lawlor Jonathan Lawlor added a comment - Why does RsRpcServices have to be involved? Could remaining scan time not be up in RegionScanner? I think RsRpcServices needs to be involved because it has the global view of when the scan started. A particular call to RegionScanner#nextRaw may not necessarily cause a timeout, but multiple calls to RegionScanner#nextRaw must be made in order to form the ScanResponse. In other words, a timeout may not be caused by a single call to RegionScanner#nextRaw but rather the accumulated time of all calls necessary to form the ScanResponse.
            stack Michael Stack added a comment -

            jonathan.lawlor Doesn't RegionScanner get created when the scan starts? Can it not run the timer?

            stack Michael Stack added a comment - jonathan.lawlor Doesn't RegionScanner get created when the scan starts? Can it not run the timer?

            Doesn't RegionScanner get created when the scan starts? Can it not run the timer?

            That's true, the RegionScanner is created when the scan starts. It also remains open Server-side as long as the Client does not close it (as is the case in non-small scans). To keep all accounting of the timer within the RegionScanner we could add a call to RegionScanner#updateTimeoutTimestamp (or something along those lines) into RsRpcServices each time we either create or retrieve the RegionScanner (would avoid accounting for timeouts within RsRpcServices). The timestamp would need to be updated on each RsRpcServices#scan call to make sure that we aren't using a previously defined timeout timestamp that would be too restrictive at this point. Then all timeout information would be communicated to RsRpcServices via the newly defined NextState from HBASE-11544.

            jonathan.lawlor Jonathan Lawlor added a comment - Doesn't RegionScanner get created when the scan starts? Can it not run the timer? That's true, the RegionScanner is created when the scan starts. It also remains open Server-side as long as the Client does not close it (as is the case in non-small scans). To keep all accounting of the timer within the RegionScanner we could add a call to RegionScanner#updateTimeoutTimestamp (or something along those lines) into RsRpcServices each time we either create or retrieve the RegionScanner (would avoid accounting for timeouts within RsRpcServices). The timestamp would need to be updated on each RsRpcServices#scan call to make sure that we aren't using a previously defined timeout timestamp that would be too restrictive at this point. Then all timeout information would be communicated to RsRpcServices via the newly defined NextState from HBASE-11544 .
            stack Michael Stack added a comment -

            That sounds good. A reset or a new session because there may be other stuff the RegionScanner wants to reset on session start other than just timers.

            stack Michael Stack added a comment - That sounds good. A reset or a new session because there may be other stuff the RegionScanner wants to reset on session start other than just timers.

            Sounds ok to me too, will be interested to see the details in a patch.

            apurtell Andrew Kyle Purtell added a comment - Sounds ok to me too, will be interested to see the details in a patch.

            Attached is a rough work in progress patch. The patch does provide a working implementation of heartbeats but I believe it could be refined so I am looking to get some feedback.

            The implementation points that I wanted to highlight for discussion are below:

            • We wanted to move all time tracking into RegionScanner and StoreScanner and leave RSRpcServices unscathed. I started off with that intention but it was slowly revealed that it may be better to simply have a timeLimit field in the call to nextRaw from RSRpcServices. Logic outlined below:
              • While it is certainly possible to add a reset() or newSession() method to the RegionScanner interface that would allow us to reset time tracking, the issue becomes how do we communicate that size limit down from the RegionScanner into the StoreScanner (the scanner that is looping through the cells for a particular column family).
              • The StoreScanners are stored in a KeyValueHeap in the RegionScanner... So it would be possible to loop through them all and call a similar reset/newSession method on all of them but that seems dirty and wasteful. It seems more appropriate to communicate the timeLimit down to only the relevant storeScanner via a timeLimit field in the InternalScanner#next(List<Cell> results, ..., timeLimit) call.
              • Since the RegionScanner also implements the InternalScanner interface, that same next method would need to be implemented in RegionScannerImpl. Because of this, I think it makes the most sense to simply have a nextRaw(List<Cell>, ..., timeLimit) method to specify the timeLimit from RSRpcServices rather than an update/newSession call
            • To avoid polluting the returned Result array with state information about heartbeats, a new heartbeat flag has been added to the ScanResponse. Since only the ScannerCallable ever sees the ScanResponse returned from the server, I have exposed the method ScannerCallable#isHeartbeatMessage() to allow the ClientScanner to check if the most recent server response was a heartbeat/keep-alive message.
            • The method postHeapNext(List<Cells>) was added to RegionScannerImpl to allow me to insert delays in between fetches of column family cells for testing. It didn't feel clean, so I was wondering if anyone had any ideas about alternative approaches to emulate long running scans on the server side
            • Since heartbeat messages have the potential to create partial results (in the event that the timeout occurs in the middle of a row) we only allow heartbeat messages if the client has specified that heartbeats are supported AND partial results are also supported.

            Ideas for improvement:

            • As earlier discussion indicated, the tracking of limits in RSRpcServices is somewhat messy. When a new limit needs to be added, the RegionScanner and InternalScanner interfaces must both be changed. The limit logic may be simplified by defining something along the lines of a ScannerLimit object. The object would have a field per limit and would have an associated Builder that would allow us to specify only the limits we care about (if a limit is not set, then it doesn't get enforced). Then, in the future, if a new limit was needed it would only amount to adding a new field in ScannerLimit and adding the appropriate enforcement logic (no changes to interfaces necessary). What do you guys think? I thought this would clean things up a bit but wanted to see if any objections first

            Of course the finer implementation points can be seen in the patch itself and any feedback would be appreciated. Will post to reviewboard

            Thanks

            jonathan.lawlor Jonathan Lawlor added a comment - Attached is a rough work in progress patch. The patch does provide a working implementation of heartbeats but I believe it could be refined so I am looking to get some feedback. The implementation points that I wanted to highlight for discussion are below: We wanted to move all time tracking into RegionScanner and StoreScanner and leave RSRpcServices unscathed. I started off with that intention but it was slowly revealed that it may be better to simply have a timeLimit field in the call to nextRaw from RSRpcServices. Logic outlined below: While it is certainly possible to add a reset() or newSession() method to the RegionScanner interface that would allow us to reset time tracking, the issue becomes how do we communicate that size limit down from the RegionScanner into the StoreScanner (the scanner that is looping through the cells for a particular column family). The StoreScanners are stored in a KeyValueHeap in the RegionScanner... So it would be possible to loop through them all and call a similar reset/newSession method on all of them but that seems dirty and wasteful. It seems more appropriate to communicate the timeLimit down to only the relevant storeScanner via a timeLimit field in the InternalScanner#next(List<Cell> results, ..., timeLimit) call. Since the RegionScanner also implements the InternalScanner interface, that same next method would need to be implemented in RegionScannerImpl. Because of this, I think it makes the most sense to simply have a nextRaw(List<Cell>, ..., timeLimit) method to specify the timeLimit from RSRpcServices rather than an update/newSession call To avoid polluting the returned Result array with state information about heartbeats, a new heartbeat flag has been added to the ScanResponse. Since only the ScannerCallable ever sees the ScanResponse returned from the server, I have exposed the method ScannerCallable#isHeartbeatMessage() to allow the ClientScanner to check if the most recent server response was a heartbeat/keep-alive message. The method postHeapNext(List<Cells>) was added to RegionScannerImpl to allow me to insert delays in between fetches of column family cells for testing. It didn't feel clean, so I was wondering if anyone had any ideas about alternative approaches to emulate long running scans on the server side Since heartbeat messages have the potential to create partial results (in the event that the timeout occurs in the middle of a row) we only allow heartbeat messages if the client has specified that heartbeats are supported AND partial results are also supported. Ideas for improvement: As earlier discussion indicated, the tracking of limits in RSRpcServices is somewhat messy. When a new limit needs to be added, the RegionScanner and InternalScanner interfaces must both be changed. The limit logic may be simplified by defining something along the lines of a ScannerLimit object. The object would have a field per limit and would have an associated Builder that would allow us to specify only the limits we care about (if a limit is not set, then it doesn't get enforced). Then, in the future, if a new limit was needed it would only amount to adding a new field in ScannerLimit and adding the appropriate enforcement logic (no changes to interfaces necessary). What do you guys think? I thought this would clean things up a bit but wanted to see if any objections first Of course the finer implementation points can be seen in the patch itself and any feedback would be appreciated. Will post to reviewboard Thanks

            Submit patch to see if anything is broken by this

            jonathan.lawlor Jonathan Lawlor added a comment - Submit patch to see if anything is broken by this
            stack Michael Stack added a comment -

            Nice writeup jonathan.lawlor

            If only timeout, then maybe premature for ScanLimit unless anything in current Scan structure that might sit better in ScanLimit?

            ....if the client has specified that heartbeats are supported AND partial results are also supported

            This might be ok for 1.1 but partials should be on all the time in 2.0.. This feature should be on all the time in 2.0. What would be the downsides if default was to allow return of partials to clients?

            On postHeapNext, yeah, ugly, but since you can't specify your own Scanner implementation serverside (you can't right?), ugly injection is all you have ... so yeah, ugly but we need it (can you make the scan latched rather than slowed....)

            When do I call isHeartbeatMessage? At want point in the processing?

            Your reasoning that new session or reset doesn't work makes sense to me.

            Will give review on patch later.

            Good stuff jonathan.lawlor

            stack Michael Stack added a comment - Nice writeup jonathan.lawlor If only timeout, then maybe premature for ScanLimit unless anything in current Scan structure that might sit better in ScanLimit? ....if the client has specified that heartbeats are supported AND partial results are also supported This might be ok for 1.1 but partials should be on all the time in 2.0.. This feature should be on all the time in 2.0. What would be the downsides if default was to allow return of partials to clients? On postHeapNext, yeah, ugly, but since you can't specify your own Scanner implementation serverside (you can't right?), ugly injection is all you have ... so yeah, ugly but we need it (can you make the scan latched rather than slowed....) When do I call isHeartbeatMessage? At want point in the processing? Your reasoning that new session or reset doesn't work makes sense to me. Will give review on patch later. Good stuff jonathan.lawlor

            Thanks for the comments stack

            If only timeout, then maybe premature for ScanLimit unless anything in current Scan structure that might sit better in ScanLimit?

            I was thinking that we could combine the batch limit, size limit, and now the time limit into ScannerLimit object. With this patch, the InternalScanner and RegionScanner interfaces now have a large cascading call structure that looks like this:

            NextState next(List<Cell> result) throws IOException;
            ...
            NextState next(List<Cell> result, int batchLimit) throws IOException;
            ...
            NextState next(List<Cell> result, int batchLimit, long sizeLimit) throws IOException;
            ...
            NextState next(List<Cell> result, int batchLimit, long sizeLimit, long timeLimit) throws IOException;
            

            As more limits are added, it gets uglier and uglier. The idea with ScannerLimit would be to change it to this:

            NextState next(List<Cell> result) throws IOException;
            ...
            NextState next(List<Cell> result, ScannerLimit limit) throws IOException;
            

            Where the ScannerLimit object can have as many limits specified as it wants (may only contain a time limit, or may contain a time limit, batch limit and size limit).

            What would be the downsides if default was to allow return of partials to clients?

            So right now partial result support is on by default but in the case that the scan is specified to be a small scan we disable partial results server side. This means that in the case of small scans we wouldn't allow heartbeat messages either since they could potentially create partials. Outside of small scans heartbeats would be supported.

            since you can't specify your own Scanner implementation serverside (you can't right?)

            As far as I can tell there is no nice way to specify your own StoreScanner implementation but upon further investigation it looks like I can specify my own KeyValueHeap implementation inside the RegionScanners. This would allow me to take this method out. Going to investigate further and see if this ugly postHeapNext method can be taken out.

            When do I call isHeartbeatMessage? At want point in the processing?

            Currently it is used inside ClientScanner.java after the Result array comes back from the server. By checking it here, we can see if the most recent response from the server (the one that returned the Results array) was a heartbeat message.

            jonathan.lawlor Jonathan Lawlor added a comment - Thanks for the comments stack If only timeout, then maybe premature for ScanLimit unless anything in current Scan structure that might sit better in ScanLimit? I was thinking that we could combine the batch limit, size limit, and now the time limit into ScannerLimit object. With this patch, the InternalScanner and RegionScanner interfaces now have a large cascading call structure that looks like this: NextState next(List<Cell> result) throws IOException; ... NextState next(List<Cell> result, int batchLimit) throws IOException; ... NextState next(List<Cell> result, int batchLimit, long sizeLimit) throws IOException; ... NextState next(List<Cell> result, int batchLimit, long sizeLimit, long timeLimit) throws IOException; As more limits are added, it gets uglier and uglier. The idea with ScannerLimit would be to change it to this: NextState next(List<Cell> result) throws IOException; ... NextState next(List<Cell> result, ScannerLimit limit) throws IOException; Where the ScannerLimit object can have as many limits specified as it wants (may only contain a time limit, or may contain a time limit, batch limit and size limit). What would be the downsides if default was to allow return of partials to clients? So right now partial result support is on by default but in the case that the scan is specified to be a small scan we disable partial results server side. This means that in the case of small scans we wouldn't allow heartbeat messages either since they could potentially create partials. Outside of small scans heartbeats would be supported. since you can't specify your own Scanner implementation serverside (you can't right?) As far as I can tell there is no nice way to specify your own StoreScanner implementation but upon further investigation it looks like I can specify my own KeyValueHeap implementation inside the RegionScanners. This would allow me to take this method out. Going to investigate further and see if this ugly postHeapNext method can be taken out. When do I call isHeartbeatMessage? At want point in the processing? Currently it is used inside ClientScanner.java after the Result array comes back from the server. By checking it here, we can see if the most recent response from the server (the one that returned the Results array) was a heartbeat message.
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12704058/HBASE-13090-v1.patch
            against master branch at commit 9c83fa7b52188d6bdfebcba75272c5c11e8b8566.
            ATTACHMENT ID: 12704058

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

            +1 tests included. The patch appears to include 20 new or modified tests.

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

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

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

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

            -1 checkstyle. The applied patch generated 1926 checkstyle errors (more than the master's current 1924 errors).

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

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

            -1 lineLengths. The patch introduces the following lines longer than 100:
            + result = result && (hasClientHandlesHeartbeatMessages() == other.hasClientHandlesHeartbeatMessages());
            + new java.lang.String[]

            { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeatMessages", }

            );
            + new java.lang.String[]

            { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", }

            );
            + public NextState next(List<Cell> outResults, int batchLimit, long sizeLimit) throws IOException {
            + NextState next(List<Cell> result, int batchLimit, long sizeLimit, long timeLimit) throws IOException;

            +1 site. The mvn site goal succeeds with this patch.

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

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//testReport/
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/checkstyle-aggregate.html

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

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704058/HBASE-13090-v1.patch against master branch at commit 9c83fa7b52188d6bdfebcba75272c5c11e8b8566. ATTACHMENT ID: 12704058 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 20 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated 1926 checkstyle errors (more than the master's current 1924 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + result = result && (hasClientHandlesHeartbeatMessages() == other.hasClientHandlesHeartbeatMessages()); + new java.lang.String[] { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeatMessages", } ); + new java.lang.String[] { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", } ); + public NextState next(List<Cell> outResults, int batchLimit, long sizeLimit) throws IOException { + NextState next(List<Cell> result, int batchLimit, long sizeLimit, long timeLimit) throws IOException; +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13206//console This message is automatically generated.

            Attaching a patch that includes the ScannerLimit idea. It does seem to make things a little nicer and presents a cleaner interface for dealing with RegionScanner#next() and InternalScanner#next when many limits need to be specified. What do you guys think?

            This patch has also managed to get rid of the ugly postHeapNext method that was included before. Now in tests we use a custom key value heap class to insert delays between column family fetches to simulate long running scans on the server side. I am still looking into how I could remove the sleeps and replace them with latches.

            Will update reviewboard with latest diff

            jonathan.lawlor Jonathan Lawlor added a comment - Attaching a patch that includes the ScannerLimit idea. It does seem to make things a little nicer and presents a cleaner interface for dealing with RegionScanner#next() and InternalScanner#next when many limits need to be specified. What do you guys think? This patch has also managed to get rid of the ugly postHeapNext method that was included before. Now in tests we use a custom key value heap class to insert delays between column family fetches to simulate long running scans on the server side. I am still looking into how I could remove the sleeps and replace them with latches. Will update reviewboard with latest diff
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12704509/HBASE-13090-v2.patch
            against master branch at commit e60cae0500daf3c146e10d808c5070c6cb24ecec.
            ATTACHMENT ID: 12704509

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

            +1 tests included. The patch appears to include 28 new or modified tests.

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

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

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

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

            -1 checkstyle. The applied patch generated 1918 checkstyle errors (more than the master's current 1917 errors).

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

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

            -1 lineLengths. The patch introduces the following lines longer than 100:
            + result = result && (hasClientHandlesHeartbeatMessages() == other.hasClientHandlesHeartbeatMessages());
            + new java.lang.String[]

            { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeatMessages", }

            );
            + new java.lang.String[]

            { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", }

            );
            + || !Bytes.equals(row, offset, length, matcher.row, matcher.rowOffset, matcher.rowLength)) {
            + public ScanResponse scan(RpcController controller, ScanRequest request) throws ServiceException {
            + public HeartbeatReversedKVHeap(List<? extends KeyValueScanner> scanners, KVComparator comparator)

            +1 site. The mvn site goal succeeds with this patch.

            -1 core tests. The patch failed these unit tests:

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//testReport/
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/checkstyle-aggregate.html

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

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704509/HBASE-13090-v2.patch against master branch at commit e60cae0500daf3c146e10d808c5070c6cb24ecec. ATTACHMENT ID: 12704509 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 28 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated 1918 checkstyle errors (more than the master's current 1917 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + result = result && (hasClientHandlesHeartbeatMessages() == other.hasClientHandlesHeartbeatMessages()); + new java.lang.String[] { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeatMessages", } ); + new java.lang.String[] { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", } ); + || !Bytes.equals(row, offset, length, matcher.row, matcher.rowOffset, matcher.rowLength)) { + public ScanResponse scan(RpcController controller, ScanRequest request) throws ServiceException { + public HeartbeatReversedKVHeap(List<? extends KeyValueScanner> scanners, KVComparator comparator) +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13232//console This message is automatically generated.
            • Fix checkstyle, line lengths, and some whitespace
            jonathan.lawlor Jonathan Lawlor added a comment - Fix checkstyle, line lengths, and some whitespace

            resubmit patch; pre-commit build didn't statrt

            jonathan.lawlor Jonathan Lawlor added a comment - resubmit patch; pre-commit build didn't statrt
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12704530/HBASE-13090-v3.patch
            against master branch at commit e60cae0500daf3c146e10d808c5070c6cb24ecec.
            ATTACHMENT ID: 12704530

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

            +1 tests included. The patch appears to include 28 new or modified tests.

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

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

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

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

            +1 checkstyle. The applied patch does not increase the total number of checkstyle errors

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

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

            -1 lineLengths. The patch introduces the following lines longer than 100:
            + result = result && (hasClientHandlesHeartbeatMessages() == other.hasClientHandlesHeartbeatMessages());
            + new java.lang.String[]

            { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeatMessages", }

            );
            + new java.lang.String[]

            { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", }

            );

            +1 site. The mvn site goal succeeds with this patch.

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

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//testReport/
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/checkstyle-aggregate.html

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

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12704530/HBASE-13090-v3.patch against master branch at commit e60cae0500daf3c146e10d808c5070c6cb24ecec. ATTACHMENT ID: 12704530 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 28 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not increase the total number of checkstyle errors +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + result = result && (hasClientHandlesHeartbeatMessages() == other.hasClientHandlesHeartbeatMessages()); + new java.lang.String[] { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeatMessages", } ); + new java.lang.String[] { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", } ); +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13237//console This message is automatically generated.
            heliangliang He Liangliang added a comment -

            Similar to HBASE-13215. It's nice to piggyback the current scanner position in the heartbeat when the limit is reached.

            heliangliang He Liangliang added a comment - Similar to HBASE-13215 . It's nice to piggyback the current scanner position in the heartbeat when the limit is reached.

            A heartbeat message is very similar to the typical response from the server with the following two exceptions:
            1. The heartbeat message will be tagged with the heartbeat flag in the ScanResponse
            2. The heartbeat message may contain an empty Result array when the region on the server has not been exhausted (i.e. there are still elements to be scanned in the current region)

            Scanners currently track their position by saving lastResult, and this mechanism will continue to work as expected with heartbeats since heartbeats ensure that we receive a Result back from the server before we return anything to the application layer.

            jonathan.lawlor Jonathan Lawlor added a comment - A heartbeat message is very similar to the typical response from the server with the following two exceptions: 1. The heartbeat message will be tagged with the heartbeat flag in the ScanResponse 2. The heartbeat message may contain an empty Result array when the region on the server has not been exhausted (i.e. there are still elements to be scanned in the current region) Scanners currently track their position by saving lastResult, and this mechanism will continue to work as expected with heartbeats since heartbeats ensure that we receive a Result back from the server before we return anything to the application layer.
            larsh Lars Hofhansl added a comment -

            Great. +1

            larsh Lars Hofhansl added a comment - Great. +1
            eshcar Eshcar Hillel added a comment -

            Could be useful to return a non empty result array even when the region is not exhausted. For example, if the scanner is async (HBASE-13071) the application can start iterating over the results instead of waiting for the server to collect the entire batch.

            eshcar Eshcar Hillel added a comment - Could be useful to return a non empty result array even when the region is not exhausted. For example, if the scanner is async ( HBASE-13071 ) the application can start iterating over the results instead of waiting for the server to collect the entire batch.
            eshcar Eshcar Hillel added a comment -

            Could be useful to return a non empty result array even when the region is not exhausted. For example, if the scanner is async (HBASE-13071) the application can start iterating over the results instead of waiting for the server to collect the entire batch.

            eshcar Eshcar Hillel added a comment - Could be useful to return a non empty result array even when the region is not exhausted. For example, if the scanner is async ( HBASE-13071 ) the application can start iterating over the results instead of waiting for the server to collect the entire batch.

            eshcar Actually, that is how it works (sorry, I was explicitly clear). When the time limit is reached the server will return to the client whatever it has accumulated thus far in a heartbeat message. What I meant by #2 is that it is possible (in the case of aggressive filtering) that when the time limit is reached, the server hasn't had a chance to accumulate ANY Results. In such a case, the Result array returned to the client would be empty

            jonathan.lawlor Jonathan Lawlor added a comment - eshcar Actually, that is how it works (sorry, I was explicitly clear). When the time limit is reached the server will return to the client whatever it has accumulated thus far in a heartbeat message. What I meant by #2 is that it is possible (in the case of aggressive filtering) that when the time limit is reached, the server hasn't had a chance to accumulate ANY Results. In such a case, the Result array returned to the client would be empty

            edit: was not* clear

            jonathan.lawlor Jonathan Lawlor added a comment - edit: was not* clear
            • Updated patch to reflect RB feedback
            jonathan.lawlor Jonathan Lawlor added a comment - Updated patch to reflect RB feedback
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12705648/HBASE-13090-v4.patch
            against master branch at commit 27cf749af884edae55454c885c7fb066f0a33c79.
            ATTACHMENT ID: 12705648

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

            +1 tests included. The patch appears to include 32 new or modified tests.

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

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

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

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

            -1 checkstyle. The applied patch generated 1919 checkstyle errors (more than the master's current 1917 errors).

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

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

            -1 lineLengths. The patch introduces the following lines longer than 100:
            + new java.lang.String[]

            { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeats", }

            );
            + new java.lang.String[]

            { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", }

            );
            + public NextState next(List<Cell> outResults, int batchLimit, long sizeLimit) throws IOException {

            +1 site. The mvn site goal succeeds with this patch.

            -1 core tests. The patch failed these unit tests:
            org.apache.hadoop.hbase.regionserver.TestHRegion

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//testReport/
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/checkstyle-aggregate.html

            Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/patchJavadocWarnings.txt
            Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12705648/HBASE-13090-v4.patch against master branch at commit 27cf749af884edae55454c885c7fb066f0a33c79. ATTACHMENT ID: 12705648 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 32 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 5 warning messages. -1 checkstyle . The applied patch generated 1919 checkstyle errors (more than the master's current 1917 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + new java.lang.String[] { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeats", } ); + new java.lang.String[] { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", } ); + public NextState next(List<Cell> outResults, int batchLimit, long sizeLimit) throws IOException { +1 site . The mvn site goal succeeds with this patch. -1 core tests . The patch failed these unit tests: org.apache.hadoop.hbase.regionserver.TestHRegion Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/checkstyle-aggregate.html Javadoc warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13319//console This message is automatically generated.
            • Fix test run (incorrect setting of joinedHeap batch limit resolved)
            • Fix javadoc warnings
            jonathan.lawlor Jonathan Lawlor added a comment - Fix test run (incorrect setting of joinedHeap batch limit resolved) Fix javadoc warnings
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12705727/HBASE-13090-v6.patch
            against master branch at commit 0d766544166fc9630bb00ae14a4a34a69d93f127.
            ATTACHMENT ID: 12705727

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

            +1 tests included. The patch appears to include 32 new or modified tests.

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

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

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

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

            -1 checkstyle. The applied patch generated 1918 checkstyle errors (more than the master's current 1917 errors).

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

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

            -1 lineLengths. The patch introduces the following lines longer than 100:
            + new java.lang.String[]

            { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeats", }

            );
            + new java.lang.String[]

            { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", }

            );

            +1 site. The mvn site goal succeeds with this patch.

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

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//testReport/
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
            Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/checkstyle-aggregate.html

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

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12705727/HBASE-13090-v6.patch against master branch at commit 0d766544166fc9630bb00ae14a4a34a69d93f127. ATTACHMENT ID: 12705727 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 32 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated 1918 checkstyle errors (more than the master's current 1917 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + new java.lang.String[] { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeats", } ); + new java.lang.String[] { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "HeartbeatMessage", } ); +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-rest.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/newPatchFindbugsWarningshbase-annotations.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13322//console This message is automatically generated.

            Updated patch incorporating feedback from reviewboard

            jonathan.lawlor Jonathan Lawlor added a comment - Updated patch incorporating feedback from reviewboard
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12725996/HBASE-13090-v7.patch
            against master branch at commit e08ef99e3042767eaf2d11adae783674acfdddeb.
            ATTACHMENT ID: 12725996

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

            +1 tests included. The patch appears to include 16 new or modified tests.

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0)

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

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

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

            -1 checkstyle. The applied patch generated 1911 checkstyle errors (more than the master's current 1910 errors).

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

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

            -1 lineLengths. The patch introduces the following lines longer than 100:
            + new java.lang.String[]

            { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeats", }

            );
            + new java.lang.String[]

            { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "MoreResultsInRegion", "HeartbeatMessage", }

            );
            + public synchronized boolean next(List<Cell> outResults, ScannerContext scannerContext) throws IOException {

            +1 site. The mvn site goal succeeds with this patch.

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

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13727//testReport/
            Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13727//artifact/patchprocess/newFindbugsWarnings.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13727//artifact/patchprocess/checkstyle-aggregate.html

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

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12725996/HBASE-13090-v7.patch against master branch at commit e08ef99e3042767eaf2d11adae783674acfdddeb. ATTACHMENT ID: 12725996 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 16 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.1 2.5.2 2.6.0) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated 1911 checkstyle errors (more than the master's current 1910 errors). +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + new java.lang.String[] { "Region", "Scan", "ScannerId", "NumberOfRows", "CloseScanner", "NextCallSeq", "ClientHandlesPartials", "ClientHandlesHeartbeats", } ); + new java.lang.String[] { "CellsPerResult", "ScannerId", "MoreResults", "Ttl", "Results", "Stale", "PartialFlagPerResult", "MoreResultsInRegion", "HeartbeatMessage", } ); + public synchronized boolean next(List<Cell> outResults, ScannerContext scannerContext) throws IOException { +1 site . The mvn site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/13727//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/13727//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/13727//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/13727//console This message is automatically generated.
            stack Michael Stack added a comment -

            +1 carried over from rb. This is a beautiful patch. I will commit later today. The long line is from generated code and is likely cause of the checkstyle complaint. I'll check it out on commit (and do any fixup if needed).

            stack Michael Stack added a comment - +1 carried over from rb. This is a beautiful patch. I will commit later today. The long line is from generated code and is likely cause of the checkstyle complaint. I'll check it out on commit (and do any fixup if needed).
            stack Michael Stack added a comment -

            Want to do a release note jonathan.lawlor?

            I wanted to put this in hbase 1.1 because it so sweet but jonathan.lawlor won't let me... says too much change too close to release.... it might be a bit 'risky'. Ahem.

            So, marking 1.2. and 2.0.

            stack Michael Stack added a comment - Want to do a release note jonathan.lawlor ? I wanted to put this in hbase 1.1 because it so sweet but jonathan.lawlor won't let me... says too much change too close to release.... it might be a bit 'risky'. Ahem. So, marking 1.2. and 2.0.
            ndimiduk Nick Dimiduk added a comment -

            I wanted to put this in hbase 1.1 because it so sweet but jonathan.lawlor won't let me... says too much change too close to release.... it might be a bit 'risky'. Ahem.

            That's a shame; this seems very important for Phoenix users who are pushing the line on OLAP-kinds of queries. What can we do to raise your confidence in the patch for branch-1.1? IT tests? CM?

            ndimiduk Nick Dimiduk added a comment - I wanted to put this in hbase 1.1 because it so sweet but jonathan.lawlor won't let me... says too much change too close to release.... it might be a bit 'risky'. Ahem. That's a shame; this seems very important for Phoenix users who are pushing the line on OLAP-kinds of queries. What can we do to raise your confidence in the patch for branch-1.1? IT tests? CM?

            ndimiduk I believe the change is solid. Just figured with branch-1.1 release so close may be a bit 'risky' to stick such a large change in right before release. While the unit tests added do stress the relevant code paths, it would be nice to run it against a workload that was having timeout problems before to prove its worth

            jonathan.lawlor Jonathan Lawlor added a comment - ndimiduk I believe the change is solid. Just figured with branch-1.1 release so close may be a bit 'risky' to stick such a large change in right before release. While the unit tests added do stress the relevant code paths, it would be nice to run it against a workload that was having timeout problems before to prove its worth
            stack Michael Stack added a comment -

            Pushed to branch-1, branch-1.1, and master. ndimiduk I pushed it since you seem to want it. Patch looks good to me and we'll be running tests over next week or so so if issue it'll turn up... Thanks.

            Thanks for the patch jonathan.lawlor

            stack Michael Stack added a comment - Pushed to branch-1, branch-1.1, and master. ndimiduk I pushed it since you seem to want it. Patch looks good to me and we'll be running tests over next week or so so if issue it'll turn up... Thanks. Thanks for the patch jonathan.lawlor
            yuzhihong@gmail.com Ted Yu added a comment -

            I got the following compilation error on branch-1:

            [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.5.1:testCompile (default-testCompile) on project hbase-server: Compilation failure
            [ERROR] /Users/tyu/1-hbase/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java:[349,11] error: unreported exception InterruptedException; must be caught or declared to be thrown
            

            Attached addendum fixes the compilation.

            yuzhihong@gmail.com Ted Yu added a comment - I got the following compilation error on branch-1: [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:2.5.1:testCompile ( default -testCompile) on project hbase-server: Compilation failure [ERROR] /Users/tyu/1-hbase/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java:[349,11] error: unreported exception InterruptedException; must be caught or declared to be thrown Attached addendum fixes the compilation.
            hudson Hudson added a comment -

            FAILURE: Integrated in HBase-1.1 #407 (See https://builds.apache.org/job/HBase-1.1/407/)
            HBASE-13090 Progress heartbeats for long running scanners (stack: rev 43f24db82566818d02062466ac421d86ddb735d8)

            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
            • hbase-common/src/main/resources/hbase-default.xml
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java
            • hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
            • hbase-protocol/src/main/protobuf/Client.proto
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/client/ClientSideRegionScanner.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/TestPartialResultsFromClientSide.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
            hudson Hudson added a comment - FAILURE: Integrated in HBase-1.1 #407 (See https://builds.apache.org/job/HBase-1.1/407/ ) HBASE-13090 Progress heartbeats for long running scanners (stack: rev 43f24db82566818d02062466ac421d86ddb735d8) hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java hbase-common/src/main/resources/hbase-default.xml hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-protocol/src/main/protobuf/Client.proto hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java hbase-server/src/main/java/org/apache/hadoop/hbase/client/ClientSideRegionScanner.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java hbase-server/src/test/java/org/apache/hadoop/hbase/TestPartialResultsFromClientSide.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
            hudson Hudson added a comment -

            FAILURE: Integrated in HBase-1.2 #2 (See https://builds.apache.org/job/HBase-1.2/2/)
            HBASE-13090 Progress heartbeats for long running scanners (stack: rev a4f77d49a5ae347c78e3d5934c4fc005d3914cb1)

            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
            • hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/TestPartialResultsFromClientSide.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java
            • hbase-protocol/src/main/protobuf/Client.proto
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/client/ClientSideRegionScanner.java
            • hbase-common/src/main/resources/hbase-default.xml
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java
            hudson Hudson added a comment - FAILURE: Integrated in HBase-1.2 #2 (See https://builds.apache.org/job/HBase-1.2/2/ ) HBASE-13090 Progress heartbeats for long running scanners (stack: rev a4f77d49a5ae347c78e3d5934c4fc005d3914cb1) hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java hbase-server/src/test/java/org/apache/hadoop/hbase/TestPartialResultsFromClientSide.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java hbase-protocol/src/main/protobuf/Client.proto hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java hbase-server/src/main/java/org/apache/hadoop/hbase/client/ClientSideRegionScanner.java hbase-common/src/main/resources/hbase-default.xml hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java

            tedyu Thanks for catching that. Seems HRegionServer no longer throws InterruptedException in master. Addendum lgtm.

            jonathan.lawlor Jonathan Lawlor added a comment - tedyu Thanks for catching that. Seems HRegionServer no longer throws InterruptedException in master. Addendum lgtm.
            yuzhihong@gmail.com Ted Yu added a comment -

            Integrated addendum to branch-1 and branch-1.1

            Thanks for taking a look, Jonathan

            yuzhihong@gmail.com Ted Yu added a comment - Integrated addendum to branch-1 and branch-1.1 Thanks for taking a look, Jonathan
            hudson Hudson added a comment -

            SUCCESS: Integrated in HBase-TRUNK #6387 (See https://builds.apache.org/job/HBase-TRUNK/6387/)
            HBASE-13090 Progress heartbeats for long running scanners (stack: rev abe3796a9907485c875932caa5f1c82071495c0f)

            • hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java
            • hbase-protocol/src/main/protobuf/Client.proto
            • hbase-common/src/main/resources/hbase-default.xml
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/TestPartialResultsFromClientSide.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/client/ClientSideRegionScanner.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java
            • hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java
            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
            hudson Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #6387 (See https://builds.apache.org/job/HBase-TRUNK/6387/ ) HBASE-13090 Progress heartbeats for long running scanners (stack: rev abe3796a9907485c875932caa5f1c82071495c0f) hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallable.java hbase-protocol/src/main/protobuf/Client.proto hbase-common/src/main/resources/hbase-default.xml hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java hbase-client/src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java hbase-server/src/test/java/org/apache/hadoop/hbase/TestPartialResultsFromClientSide.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ScannerContext.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/ClientScanner.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/compactions/TestStripeCompactionPolicy.java hbase-server/src/main/java/org/apache/hadoop/hbase/client/ClientSideRegionScanner.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/NoLimitScannerContext.java hbase-client/src/main/java/org/apache/hadoop/hbase/client/ScannerCallableWithReplicas.java hbase-protocol/src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
            hudson Hudson added a comment -

            FAILURE: Integrated in HBase-1.1 #408 (See https://builds.apache.org/job/HBase-1.1/408/)
            HBASE-13090 Addendum fixes compilation error in TestScannerHeartbeatMessages (tedyu: rev 7b84d7d7812cffb7da3ccfb40123dc43f18e594c)

            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
            hudson Hudson added a comment - FAILURE: Integrated in HBase-1.1 #408 (See https://builds.apache.org/job/HBase-1.1/408/ ) HBASE-13090 Addendum fixes compilation error in TestScannerHeartbeatMessages (tedyu: rev 7b84d7d7812cffb7da3ccfb40123dc43f18e594c) hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
            hudson Hudson added a comment -

            FAILURE: Integrated in HBase-1.2 #3 (See https://builds.apache.org/job/HBase-1.2/3/)
            HBASE-13090 Addendum fixes compilation error in TestScannerHeartbeatMessages (tedyu: rev b655a9909e1f5ed2823d0adfd1f42d5af5017dd1)

            • hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
            hudson Hudson added a comment - FAILURE: Integrated in HBase-1.2 #3 (See https://builds.apache.org/job/HBase-1.2/3/ ) HBASE-13090 Addendum fixes compilation error in TestScannerHeartbeatMessages (tedyu: rev b655a9909e1f5ed2823d0adfd1f42d5af5017dd1) hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScannerHeartbeatMessages.java
            yuzhihong@gmail.com Ted Yu added a comment -

            TestScannerHeartbeatMessages fails in branch-1
            e.g. https://builds.apache.org/job/HBase-1.1/411/testReport/org.apache.hadoop.hbase.regionserver/TestScannerHeartbeatMessages/testScannerHeartbeatMessages/

            In TestScannerHeartbeatMessages#testImportanceOfHeartbeats(), there was no exception raised when heartbeatsEnabled was set to false:

                HeartbeatRPCServices.heartbeatsEnabled = false;
                try {
                  testCallable.call();
                } catch (Exception e) {
                  return;
            

            Debugging to see what might be the cause.

            BTW test table should be deleted at the end of the test:

            @@ -173,6 +173,7 @@ public class TestScannerHeartbeatMessages {
            
               @AfterClass
               public static void tearDownAfterClass() throws Exception {
            +    TEST_UTIL.deleteTable(TABLE_NAME);
                 TEST_UTIL.shutdownMiniCluster();
               }
            
            

            I sometimes got the following in subsequent test run:

            	at org.apache.hadoop.hbase.regionserver.TestScannerHeartbeatMessages.createTestTable(TestScannerHeartbeatMessages.java:139)
            	at org.apache.hadoop.hbase.regionserver.TestScannerHeartbeatMessages.setUpBeforeClass(TestScannerHeartbeatMessages.java:134)
            Caused by: org.apache.hadoop.ipc.RemoteException: testScannerHeartbeatMessagesTable
            	at org.apache.hadoop.hbase.master.procedure.CreateTableProcedure.prepareCreate(CreateTableProcedure.java:283)
            	at org.apache.hadoop.hbase.master.procedure.CreateTableProcedure.executeFromState(CreateTableProcedure.java:106)
            	at org.apache.hadoop.hbase.master.procedure.CreateTableProcedure.executeFromState(CreateTableProcedure.java:1)
            
            yuzhihong@gmail.com Ted Yu added a comment - TestScannerHeartbeatMessages fails in branch-1 e.g. https://builds.apache.org/job/HBase-1.1/411/testReport/org.apache.hadoop.hbase.regionserver/TestScannerHeartbeatMessages/testScannerHeartbeatMessages/ In TestScannerHeartbeatMessages#testImportanceOfHeartbeats(), there was no exception raised when heartbeatsEnabled was set to false: HeartbeatRPCServices.heartbeatsEnabled = false ; try { testCallable.call(); } catch (Exception e) { return ; Debugging to see what might be the cause. BTW test table should be deleted at the end of the test: @@ -173,6 +173,7 @@ public class TestScannerHeartbeatMessages { @AfterClass public static void tearDownAfterClass() throws Exception { + TEST_UTIL.deleteTable(TABLE_NAME); TEST_UTIL.shutdownMiniCluster(); } I sometimes got the following in subsequent test run: at org.apache.hadoop.hbase.regionserver.TestScannerHeartbeatMessages.createTestTable(TestScannerHeartbeatMessages.java:139) at org.apache.hadoop.hbase.regionserver.TestScannerHeartbeatMessages.setUpBeforeClass(TestScannerHeartbeatMessages.java:134) Caused by: org.apache.hadoop.ipc.RemoteException: testScannerHeartbeatMessagesTable at org.apache.hadoop.hbase.master.procedure.CreateTableProcedure.prepareCreate(CreateTableProcedure.java:283) at org.apache.hadoop.hbase.master.procedure.CreateTableProcedure.executeFromState(CreateTableProcedure.java:106) at org.apache.hadoop.hbase.master.procedure.CreateTableProcedure.executeFromState(CreateTableProcedure.java:1)

            tedyu thanks for digging in here. I have done some investigation into the root cause of this issue and it seems to be coming from the field MIN_RPC_TIMEOUT inside RpcRetryingCaller in branch-1. This MIN_RPC_TIMEOUT field in branch-1 prevents setting the RPC timeout value to anything less than 2 seconds. In master this field no longer exists and the timeout value can be specified to be as small as we wish. In the case of TestScannerHeartbeatMessages, the RPC timeout was specified to be 0.5 seconds which is why it fails when it is 2 seconds instead. I will attach a patch shortly to address this issue, thanks!

            jonathan.lawlor Jonathan Lawlor added a comment - tedyu thanks for digging in here. I have done some investigation into the root cause of this issue and it seems to be coming from the field MIN_RPC_TIMEOUT inside RpcRetryingCaller in branch-1. This MIN_RPC_TIMEOUT field in branch-1 prevents setting the RPC timeout value to anything less than 2 seconds. In master this field no longer exists and the timeout value can be specified to be as small as we wish. In the case of TestScannerHeartbeatMessages, the RPC timeout was specified to be 0.5 seconds which is why it fails when it is 2 seconds instead. I will attach a patch shortly to address this issue, thanks!

            Filed HBASE-13514 to address the test failures in branch-1 and branch-1.1

            jonathan.lawlor Jonathan Lawlor added a comment - Filed HBASE-13514 to address the test failures in branch-1 and branch-1.1
            ndimiduk Nick Dimiduk added a comment -

            Closing issues released in 1.1.0.

            ndimiduk Nick Dimiduk added a comment - Closing issues released in 1.1.0.

            People

              jonathan.lawlor Jonathan Lawlor
              apurtell Andrew Kyle Purtell
              Votes:
              0 Vote for this issue
              Watchers:
              27 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: