HBASE-27691 Prevent filters from seeing synthetic scan start cells - #8485
Conversation
|
@apurtell @virajjasani @Apache9 Would appreciate a review when you have a moment. |
|
@junegunn Would appreciate a review when you have a moment. Thanks! |
|
I could reproduce the problem locally: java_import org.apache.hadoop.hbase.CompareOperator
java_import org.apache.hadoop.hbase.filter.BinaryComparator
java_import org.apache.hadoop.hbase.filter.RowFilter
java_import org.apache.hadoop.hbase.filter.WhileMatchFilter
create 't', 'd'
put 't', 'row1', 'd:foo', 'bar'
put 't', 'row2', 'd:foo', 'bar'
put 't', 'row3', 'd:foo', 'bar'
flush 't'
# 3 rows
scan 't', FILTER => WhileMatchFilter.new(RowFilter.new(CompareOperator::NOT_EQUAL,
BinaryComparator.new(''.to_java_bytes)))
# no rows
scan 't', FILTER => WhileMatchFilter.new(RowFilter.new(CompareOperator::NOT_EQUAL,
BinaryComparator.new(''.to_java_bytes))),
COLUMNS => ['d:foo']The history behind this issue is quite involved, so I don't feel confident making a judgment call on my own. From what I understand, this patch was first suggested by Lars Hofhansl in early 2013: He raised a concern about the performance impact of eager seeks, but the question was never answered. Adding So the question still remains. Do you have a view on the performance impact? The eager seek is once per scanner open rather than per row, and scans without explicit columns already take that path, so it may well be fine. But it applies to every filtered explicit-column non-Get scan, so we should understand the cost before making it the default. To be clear, I am not suggesting we leave the bug unfixed. Correctness should take priority over performance. I would just like to understand the cost before we commit. |
|
@junegunn Thanks for the thoughtful review. I have added another unit test based on how you reproduced the issue with the WhileMatchFilter. I also updated the description of this PR to express the potential cost concern. I do not know how to best simulate a reasonable worst-case scenario to gather concrete statistics however. Any advice? For awareness I do have another branch with a new configuration property implemented to be able to revert this behavior change in cases where the current behavior is preferable (if unset, the default is to fix this bug). |
|
Thanks, the updated description reads well. I'd like to ask the original members who started the discussion, but unfortunately, they are no longer active in this project. For unit tests, you might want to check out TestSeekOptimizations. The existing cases seem to compare lazy seeks against eager. They never set a filter though. I also find it hard to come up with a realistic scenario that would actually suffer from the regression, since it depends on several factors at once:
Regarding the new configuration property, I don't think we should add a flag to re-introduce a correctness bug unless the performance hit is really unacceptable, and we don't know that yet. But I doubt it is. Out of curiosity: How did you run into this issue? Do you have a production workload that was affected by the bug? If so, what does that workload look like? |
Sounds good from here. I will see today what corresponding updates can be made there. Update: I have added a new test case to that class.
Agreed.
Right, for that reason my team's consensus opinion is that the risk of this correction being problematic for any real-world application is extremely small.
Yes, we did. The issue was encountered in a dev environment with an existing legacy data processing workload being migrated from HBase 1.x to 2.x. It has a custom multi-segment row key binary serialization format with a RowFilter for scan efficiency (CompareOperator.EQUAL + ByteArrayComparable impl). We use the DataInputStream class to decode its segments. Reading the stream for a row ended unexpectedly when a At first we were confused how a row key could have become corrupted in such a way when encoded from its natural structured form, then noticed the value was a region start boundary. That led us to this HBase JIRA opened a few years ago. For now, our temporary mitigation is catching any Exception (and returning 1 to skip the row) as analysis showed several different possibilities if decoding arbitrary bytes. While the code is certainly more robust now with that guardrail in place, any issue where the encoding of real row keys is malformed would effectively cause quiet (apart from region server error logging) downstream data loss rather than failing fast which is what we would want. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts StoreScanner’s initial seek behavior so that server-side Filters/Comparators are never invoked with a synthetic “scan start” Cell for filtered, non-Get scans, preventing malformed row-key observations while preserving lazy seeking where safe.
Changes:
- Disable initial lazy seek for filtered non-Get scans in
StoreScanner. - Add/extend tests to ensure filters/comparators only see real persisted rows and to validate the lazy-seek decision matrix.
- Extend seek-optimization tests to assert filtered explicit-column scans always eager-seek.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java | Disables initial lazy seek when a Filter is present on a non-Get scan to avoid exposing synthetic Cells to filters. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java | Adds regression + focused tests covering comparator/filter exposure and initial lazy-seek decision branches. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestSeekOptimizations.java | Refactors setup and adds a test asserting filtered scans always eager-seek; introduces ScanResult tracking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The |
|
I extended @@ -77,10 +77,11 @@ public class TestSeekOptimizations {
private static final int PUTS_PER_ROW_COL = 50;
private static final int DELETES_PER_ROW_COL = 10;
- private static final int NUM_ROWS = 3;
+ // Single digit only: rowStr() is "row" + i, so row10 would sort before row2.
+ private static final int NUM_ROWS = 9;
private static final int NUM_COLS = 3;
- private static final boolean VERBOSE = false;
+ private static final boolean VERBOSE = true;
/**
* Disable this when this test fails hopelessly and you need to debug a simpler case.
@@ -235,7 +236,7 @@ public class TestSeekOptimizations {
columnArr.length == 0 ? "all columns" : ("columns=" + Arrays.toString(columnArr));
final String testDesc = "Bloom=" + bloomType + ", compr=" + comprAlgo + ", "
+ (scan.isGetScan() ? "Get" : "Scan") + ": " + columnRestrictionStr + ", " + rowRestrictionStr
- + ", maxVersions=" + maxVersions + ", lazySeek=" + lazySeekEnabled;
+ + ", maxVersions=" + maxVersions + ", lazySeek=" + lazySeekEnabled + ", filtered=" + filtered;
long seekCount = StoreFileScanner.getSeekCount() - initialSeekCount;
if (VERBOSE) {
System.err.println("Seek count: " + seekCount + ", KVs returned: " + actualKVs.size() + ". "
@@ -467,6 +468,12 @@ public class TestSeekOptimizations {
public void testSeeksEagerlyWhenFiltered() throws IOException {
ScanResult filteredLazyResults = testScan(new int[] { 0 }, true, 0, 2, 1, true);
ScanResult filteredEagerResults = testScan(new int[] { 0 }, false, 0, 2, 1, true);
+ // Does the extra cost scale with rows scanned? filtered=false is the behavior before
+ // this patch. endRow never equals startRow: that would be a Get, which this patch exempts.
+ for (int endRow : new int[] { 1, 2, 5, 8 }) {
+ testScan(new int[] { 0 }, true, 0, endRow, 1, false);
+ testScan(new int[] { 0 }, true, 0, endRow, 1, true);
+ }
assertKVListsEqual("Filtered explicit column scan results differ with lazy seeking enabled",
filteredEagerResults.cells, filteredLazyResults.cells);
assertEquals(filteredEagerResults.scannerOpenSeekCount,Distilling the
The fixture has 7 store files, and the numbers fit exactly: So in this fixture the eager initial seek does not cost a fixed N seeks at open. It doubles the per-row seek count for the whole scan. That contradicts the description:
and it also inverts what I said earlier about long scans amortizing the cost away. I haven't worked out what's causing the doubling, could you take a look? |
I confirmed it's the mere presence of the filter, not this patch unexpectedly changing per-row behavior. The higher seek count in this test for a filtered scan is seen with unpatched code as well. Without a filter, HBase sees the newest version and knows it can move immediately to the next row. That is why the counter is naturally lower. For this unfiltered case, it takes With a filter, HBase must be more cautious because a general filter might decide to reject the newest version and accept an older one. So in this case it ends up taking a different internal route: The patch modifies only the initial positioning, ensuring the first Cell shown to the filter is real. It doesn't change the subsequent seek/reseek logic. Please let me know if you need additional supporting evidence. |
|
You're right, thanks for looking into it. Reverting just the
That puts us back where we were before: a fixed cost bounded by the store file count, paid once per scanner open. And since any scan this patch affects already has a filter, it is already paying the per-row overhead, so the flat 6 is a small share of the total: around 20% at 2 rows, under 5% at 9, shrinking from there. +1 from me. |
…8485) Co-authored-by: Andrew Olson <aolson1@cerner.com> Signed-off-by: Junegunn Choi <junegunn@apache.org>
…8485) Co-authored-by: Andrew Olson <aolson1@cerner.com> Signed-off-by: Junegunn Choi <junegunn@apache.org>
|
Merged, thanks! Cleanly cherry-picked to branch-3 and branch-3.0. The 2.x branches needed some changes in the test code, so I opened backport PRs. I'll merge them when CI passes.
|
HBASE-27691
What changes were proposed in this pull request?
This patch disables
StoreScanner's initial lazy seek for filtered non-Get scans. Doing so prevents synthetic lazy-seek Cells from being exposed to server-side Filters and Comparators, while retaining lazy seeking for unfiltered Scans and Gets.Why are the changes needed?
A region start boundary is not guaranteed to be a valid application row key. Passing its synthetic Cell to a
RowFiltercomparator violates the Filter contract thatfilterRowKeyreceives the first actual Cell of a row and can result in unexpected exceptions when Filter / Comparator code attempts to parse a seemingly truncated or otherwise malformed row key.In our case that manifested like this:
This change reinstates the protection intended by HBASE-6562, using its later proposed (unmerged) eager initial seek patch.
Are there any concerns?
First let's provide some background context.
How does lazy seeking work, and why is its design problematic?
Lazy seeking uses synthetic Cells as part of the scanner's control flow. They are essentially placeholders indicating "start looking from here." A problem with this design is that before a real Cell has been read from a StoreFile, this placeholder is made visible to a filter with no supplemental context to be able to differentiate it as such. A filter has no definitive means to distinguish a synthetic Cell from a real one, so it may attempt to unsafely process the row key (decoding it, comparing it, using the value to decide to end the scan, etc). Filter or comparator code therefore can be required to have overly broad exception handling.
What is the proposal for fixing this design problem?
To elaborate on what was stated above, this patch ensures a filtered non-Get scan reads a real first Cell before invoking the filter. The correction is limited to this specific case where correctness requires that filters see a real Cell. Scans without explicit columns already do an eager initial seek. Unfiltered scans, Gets, and subsequent post-initialization repositioning still retain the existing lazy seek behavior.
What are the risks of making this correction?
A known risk of making this correction is potentially unnecessary work done at the beginning of the scan. A StoreScanner is the scanner for one column family in one region scan. On opening it, HBase may have multiple underlying scanners, one for each StoreFile plus one for the Region Server MemStore. Previously with an explicit-column scan, HBase could tell each underlying scanner "don't seek yet - wait until I know you are needed." The patch instead says that filtered non-Get scans must "seek now, so the first Cell I present to the filter is real." If a column family has many StoreFiles, each selected StoreFile scanner may do an HFile index lookup and potentially read blocks to position itself. The MemStore scanner is also positioned. The cost is proportional to the number of underlying scanners opened, not the number of rows returned. That overhead can materially matter for short filtered scans over stores with many files if the scan would otherwise have avoided touching some of them.
Is there a better way to fix this?
I have been unable to determine a more appropriate way to address this and am open to ideas. Deeper API design changes could involve enhancing the Filter abstract class or Cell interface and do not appear advisable for such a narrow problem surface area, as that would introduce major risks.
How was this patch tested?
A ROWCOL Bloom regression test verifies that a Comparator sees only the persisted row, not a synthetic region-boundary Cell. Focused tests also cover each lazy-seek decision branch.
mvn -ntp -pl hbase-server -am \ -Dtest=TestScanner \ -Dsurefire.failIfNoSpecifiedTests=false \ test