Uh oh!
There was an error while loading. Please reload this page.
HBASE-26122: Implement an optional maximum size for Gets, after which a partial result is returned - #3532
Merged
Conversation
bbeaudreaultforce-pushed
the
max_get_size_upstream
branch
2 times, most recently
from
July 27, 2021 11:57
b72fade to
3ef4560CompareApache-HBase
commented
Jul 27, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2021
🎊 +1 overall
This message was automatically generated. |
bbeaudreaultforce-pushed
the
max_get_size_upstream
branch
from
July 28, 2021 20:52
3ef4560 to
8a43862CompareApache-HBase
commented
Jul 28, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 28, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2021
💔 -1 overall
This message was automatically generated. |
bbeaudreaultforce-pushed
the
max_get_size_upstream
branch
from
July 29, 2021 11:26
8a43862 to
bef5299CompareApache-HBase
commented
Jul 29, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2021
💔 -1 overall
This message was automatically generated. |
bbeaudreaultforce-pushed
the
max_get_size_upstream
branch
2 times, most recently
from
July 29, 2021 19:44
f362fda to
3fc6c7cCompareApache-HBase
commented
Jul 29, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 29, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 30, 2021
🎊 +1 overall
This message was automatically generated. |
bbeaudreaultforce-pushed
the
max_get_size_upstream
branch
from
August 5, 2021 12:17
3fc6c7c to
5d9c8cbCompareApache-HBase
commented
Aug 5, 2021
🎊 +1 overall
This message was automatically generated. |
… a partial result is returned
bbeaudreaultforce-pushed
the
max_get_size_upstream
branch
from
August 5, 2021 15:07
5d9c8cb to
4ab2b9aCompareApache-HBase
commented
Aug 5, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2021
🎊 +1 overall
This message was automatically generated. |
saintstack
left a comment
Contributor
There was a problem hiding this comment.
LGTM.
Can only go into branch-2 and master I'd say, not into 2.4 given it adds API. That ok by you @bbeaudreault ?
| * | ||
| * If set to a value greater than zero, the server may respond with a Result where | ||
| * {@link Result#mayHaveMoreCellsInRow()} is true. The user is required to handle | ||
| * this case. |
Contributor
There was a problem hiding this comment.
What do you do when mayHaveMoreCellsInRow is true @bbeaudreault ? How do you use this boolean in prod (if you don't mind me asking...)
ContributorAuthor
There was a problem hiding this comment.
At HubSpot we have a wrapper implementation of Table which all downstream users go through. This wrapper table enforces that setMaxResultSize is set to a standard value that we've deemed safe. If a result comes back and mayHaveMoreCellsInRow is true, we throw an exception. If a team gets such an exception they can request a temporary allowance which disables the check. In the meantime they are expected to add a filter to paginate so they don't hit the max limit.
This is a little draconian, but we used to have lots of OOM issues due to large gets/puts/scans. Another possible solution is to iterate with PageFilter, like I did in testGetPartialResults. We planned to do something like that eventually, but in the end we had rolled this out in such a way that the number of exceptions were so few that we never did the work.
Would you be open to an automatic stitching in the future, like we do with Scans? I can't do that now, but might be a reasonable followup jira.
ContributorAuthor
There was a problem hiding this comment.
In terms of that last sentence, maybe it's better to not support stitching for Gets. Instead people should rewrite these large Gets as Scans or add filters like above. Stitching obviously increases the latency and that could be very misleading for Gets. Multigets even worse (and harder to implement)
Contributor
There was a problem hiding this comment.
I want a Cell Streaming API.
| repeated ColumnFamilyTimeRange cf_time_range = 13; | ||
| optional bool load_column_families_on_demand = 14; /* DO NOT add defaults to load_column_families_on_demand. */ | ||
| optional uint64 max_result_size = 15; |
Contributor
There was a problem hiding this comment.
FYI, these protos we want to let go of eventually. They are for use of downstreamers, not for internal hbase use.... internally we use the shaded stuff. So, its fine adding this here but probably better not adding it.
Uh oh!
There was an error while loading. Please reload this page.
| private Pair<List<Cell>, ScannerContext> getInternal(Get get, boolean withCoprocessor, long nonceGroup, long nonce) | ||
| throws IOException { | ||
| ScannerContext scannerContext = ScannerContext.newBuilder() |
Contributor
There was a problem hiding this comment.
Every Get will now carry a ScannerContext where previous it was usually null? (IIRC) Do you think this will cost Bryan? Will there be other benefits having a ScannerContext on every Get?
ContributorAuthor
There was a problem hiding this comment.
This is a good point, thanks. I'll push a commit which only adds a ScannerContext if getMaxResultSize > 0
ContributorAuthor
There was a problem hiding this comment.
With the new commit, I also removed the ScannerContext from the version of get which just returns a List<Cell>. I probably shouldn't have included it in the first place. It's odd to return a partial result with no way to inform the client to that fact. I'm not sure in what context this method is used, looks like just tests maybe if intellij isn't lying.
Apache-HBase
commented
Aug 10, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 10, 2021
🎊 +1 overall
This message was automatically generated. |
saintstack
approved these changes
Aug 11, 2021
| ? ScannerContext.newBuilder() | ||
| .setSizeLimit(LimitScope.BETWEEN_CELLS, get.getMaxResultSize(), get.getMaxResultSize()) | ||
| .build() | ||
| : null; |
saintstack
commented
Aug 11, 2021
Contributor
[2021-08-10T22:31:00.190Z] +1 overall Fail is our build issue... java.nio.file.FileSystemException: /home/jenkins/jenkins-agent/workspace/Base-PreCommit-GitHub-PR_PR-3532/yetus-jdk11-hadoop3-check: Read-only file system |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/HBASE-26122
Adds a
Get#setMaxResultSize(long)method, similar to the one in Scan. The default is -1, meaning not enabled. When a max result size is added to a Get and there are more cells than fit in the specified size,Result#mayHaveMoreCellsInRow()will be true. This utilizes ScannerContext on the server side, since Gets are backed by single row scans.Unlike Scans, no response stitching is implemented. The user must handle the possible true value in
Result#mayHaveMoreCellsInRow()accordingly. This seems like a fine initial behavior since the default is unlimited, meaning a user would have to opt in to this new functionality with the intent to handle the possible return values.I've added tests to TestHRegion for the HRegion implementation. For the RSRpcServices I wasn't sure of the best convention, so I added it to the existing TestPartialResultsFromClientSide. All new tests pass.