HBASE-26122: Implement an optional maximum size for Gets, after which a partial result is returned #3532
Conversation
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
bbeaudreault
force-pushed
the
max_get_size_upstream
branch
2 times, most recently
from
b72fade to
3ef4560
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
max_get_size_upstream
branch
from
3ef4560 to
8a43862
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
max_get_size_upstream
branch
from
8a43862 to
bef5299
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
max_get_size_upstream
branch
2 times, most recently
from
f362fda to
3fc6c7c
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
max_get_size_upstream
branch
from
3fc6c7c to
5d9c8cb
Compare
|
🎊 +1 overall
This message was automatically generated. |
… a partial result is returned
bbeaudreault
force-pushed
the
max_get_size_upstream
branch
from
5d9c8cb to
4ab2b9a
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Aug 10, 2021
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. |
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...)
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.
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)
There was a problem hiding this comment.
I want a Cell Streaming API.
| @@ -91,6 +91,8 @@ message Get { | |||
| optional Consistency consistency = 12 [default = STRONG]; | |||
| 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; | |||
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.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
|
|
||
| private Pair<List<Cell>, ScannerContext> getInternal(Get get, boolean withCoprocessor, long nonceGroup, long nonce) | ||
| throws IOException { | ||
| ScannerContext scannerContext = ScannerContext.newBuilder() |
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?
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
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.
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +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; |
|
[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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.