HBASE-27691 Prevent filters from seeing synthetic scan start cells - #8485
HBASE-27691 Prevent filters from seeing synthetic scan start cells#8485noslowerdna wants to merge 3 commits into
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). |
2 participants
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