HBASE-27691 Prevent filters from seeing synthetic scan start cells

[noslowerdna]

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 RowFilter comparator violates the Filter contract that filterRowKey receives 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:

Caused by: java.lang.RuntimeException: java.io.EOFException
	at com.package.MyComparator.compareTo(MyComparator.java:133)
	at org.apache.hadoop.hbase.PrivateCellUtil.compareRow(PrivateCellUtil.java:1240)
	at org.apache.hadoop.hbase.filter.CompareFilter.compareRow(CompareFilter.java:148)
	at org.apache.hadoop.hbase.filter.RowFilter.filterRowKey(RowFilter.java:90)
	at org.apache.hadoop.hbase.filter.FilterListWithOR.filterRowKey(FilterListWithOR.java:345)
	at org.apache.hadoop.hbase.filter.FilterList.filterRowKey(FilterList.java:152)
	at org.apache.hadoop.hbase.filter.FilterListWithAND.filterRowKey(FilterListWithAND.java:227)
	at org.apache.hadoop.hbase.filter.FilterList.filterRowKey(FilterList.java:152)
	at org.apache.hadoop.hbase.filter.FilterWrapper.filterRowKey(FilterWrapper.java:108)
	at org.apache.hadoop.hbase.regionserver.HRegion$RegionScannerImpl.filterRowKey(HRegion.java:7545)
	at org.apache.hadoop.hbase.regionserver.HRegion$RegionScannerImpl.nextInternal(HRegion.java:7361)
	at org.apache.hadoop.hbase.regionserver.HRegion$RegionScannerImpl.nextRaw(HRegion.java:7153)
	at org.apache.hadoop.hbase.regionserver.RSRpcServices.scan(RSRpcServices.java:3330)
	...

This change reinstates the protection intended by HBASE-6562, using its later proposed (unmerged) eager initial seek patch.

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

[noslowerdna]

@apurtell @virajjasani @Apache9 Would appreciate a review when you have a moment.

[noslowerdna]

@junegunn Would appreciate a review when you have a moment. Thanks!

[junegunn]

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:

• https://issues.apache.org/jira/browse/HBASE-6562?focusedCommentId=13543488&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13543488

He raised a concern about the performance impact of eager seeks, but the question was never answered. Adding hasFilterRowKey was briefly discussed as a way to limit the cases where the lazy seek optimization is lost, but the community never reached a consensus.

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.