HBASE-28634 Fix FuzzyRowFilter may not return data on reverse scans by PDavid · Pull Request #6457 · apache/hbase

PDavid · 2024-11-11T08:06:51Z

The FuzzyRowFilter reverse scan related code was added under HBASE-12183.

My Changes:

Reproduced the issue with a mini cluster test.
Corrected reverse unit tests in TestFuzzyRowFilter as some expectations were not correct.
Fixed hint returned by FuzzyRowFilter.getNextForFuzzyRule() in the reverse case:
- Before we only trimmed the trailing zeros from the hint in the forward scan case. From now on we also do this in the reverse case.
  - Also extracted PrefixFilter.increaseLastNonMaxByte() method to PrivateCellUtil so that it can be reused in FuzzyRowFilter.
- When the hint should contain a 0xFF. As a start, when we have a reverse scan, we not only need trailing 0xff's instead of trailing 0x00's, but it seems we always need 0xff's instead of the 0x00's.
Made some small improvements to FuzzyRowFilter which IntelliJ IDEA suggested (final fields, simplifications).

PDavid · 2024-11-13T08:21:23Z

In the latest PR build two unit tests failed:

TestMasterFailoverBalancerPersistence
TestRegionServerReportForDuty

For me, these look unrelated and I was able to successfully run these tests locally.

Reproduced the issue with a mini cluster test.

Some expectations were not correct. Also extracted PrefixFilter.increaseLastNonMaxByte() method to PrivateCellUtil so that it can be reused in FuzzyRowFilter.

…() when the hint should contain a 0xFF As a start, when we have a reverse scan, we not only need trailing 0xff's instead of trailing 0x00's, but it seems we always need 0xff's instead of the 0x00's.

…GetNextForFuzzyRuleForward

to not start another minicluster needlessly.

stoty

+1 LGTM

stoty · 2024-11-13T11:23:01Z

 @Override
 public ReturnCode filterCell(final Cell c) {
- final int startIndex = lastFoundIndex >= 0 ? lastFoundIndex : 0;
+ final int startIndex = Math.max(lastFoundIndex, 0);


I have checked, this compiles to the same inline, so it's not a performance problem.

Many thanks. 👍
Yes, this line change was not because of performance. Actually here just IntelliJ IDEA suggested to replace this to make it simpler to understand. It can be reverted if needed.

stoty · 2024-11-18T09:39:22Z

Thank you.
Please also create a backport PR for branch-2.

As the comment did not make much sense without the mask and current row.

Apache-HBase · 2024-11-20T08:48:34Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 17s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	hbaseanti	0m 0s		Patch does not have any anti-patterns.
			_ master Compile Tests _
+0 🆗	mvndep	0m 16s		Maven dependency ordering for branch
+1 💚	mvninstall	2m 56s		master passed
+1 💚	compile	4m 23s		master passed
+1 💚	checkstyle	1m 11s		master passed
+1 💚	spotbugs	2m 50s		master passed
+1 💚	spotless	0m 44s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 11s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 53s		the patch passed
+1 💚	compile	4m 26s		the patch passed
+1 💚	javac	4m 26s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 11s		the patch passed
+1 💚	spotbugs	3m 10s		the patch passed
+1 💚	hadoopcheck	10m 12s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 43s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 31s		The patch does not generate ASF License warnings.
		42m 35s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6457/6/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6457
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux d04708389e7c 5.4.0-200-generic #220-Ubuntu SMP Fri Sep 27 13:19:16 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `9cacc58`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	84 (vs. ulimit of 30000)
modules	C: hbase-common hbase-client hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6457/6/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

PDavid · 2024-11-20T08:55:41Z

Prepared a backport PR for branch-2 here: #6482

…6457) Signed-off-by: Istvan Toth <stoty@apache.org> (cherry picked from commit a735eff)

…pache#6457)

This comment has been minimized.
Sign in to view

PDavid force-pushed the HBASE-28634-FuzzyRowFilter-reverse-no-data branch from 9abb459 to ed2e4a5Compare November 12, 2024 14:30

This comment has been minimized.
Sign in to view

PDavid marked this pull request as ready for review November 13, 2024 08:21

PDavid force-pushed the HBASE-28634-FuzzyRowFilter-reverse-no-data branch from ed2e4a5 to c03e416Compare November 13, 2024 09:39

This comment has been minimized.
Sign in to view

PDavid added 8 commits November 18, 2024 09:51

HBASE-28634 Fix FuzzyRowFilter may not return data on reverse scans
bb03baa
Reproduced the issue with a mini cluster test.

HBASE-28634 Corrected reverse unit tests in TestFuzzyRowFilter
00eafcc
Some expectations were not correct. Also extracted PrefixFilter.increaseLastNonMaxByte() method to PrivateCellUtil so that it can be reused in FuzzyRowFilter.

HBASE-28634 Fixed hint returned by FuzzyRowFilter.getNextForFuzzyRule…
019f6d8
…() when the hint should contain a 0xFF As a start, when we have a reverse scan, we not only need trailing 0xff's instead of trailing 0x00's, but it seems we always need 0xff's instead of the 0x00's.

HBASE-28634 Removed unneeded comments
85fd431

HBASE-28634 Added more separating comments to TestFuzzyRowFilter.test…
719eb63
…GetNextForFuzzyRuleForward

HBASE-28634 Move reproduction test into TestFuzzyRowFilterEndToEnd
5fa2074
to not start another minicluster needlessly.

HBASE-28634 Small improvements to FuzzyRowFilter
188f4a1

HBASE-28634 Small comment improvement in FuzzyRowFilter
348b364

PDavid force-pushed the HBASE-28634-FuzzyRowFilter-reverse-no-data branch from c03e416 to 348b364Compare November 18, 2024 09:00

stoty approved these changes Nov 18, 2024
View reviewed changes

This comment has been minimized.
Sign in to view

HBASE-28634 Added more explanation to comment in unit test
9cacc58
As the comment did not make much sense without the mask and current row.

PDavid mentioned this pull request Nov 20, 2024
HBASE-28634 Fix FuzzyRowFilter may not return data on reverse scans #6482
Merged

stoty merged commit a735eff into apache:masterNov 25, 2024

stoty pushed a commit that referenced this pull request Nov 25, 2024
HBASE-28634Fix FuzzyRowFilter may not return data on reverse scans (#…
abe3e67
…6457) Signed-off-by: Istvan Toth <stoty@apache.org> (cherry picked from commit a735eff)

PDavid deleted the HBASE-28634-FuzzyRowFilter-reverse-no-data branch November 25, 2024 09:03

gvprathyusha6 pushed a commit to gvprathyusha6/hbase that referenced this pull request Dec 19, 2024
HBASE-28634 Fix FuzzyRowFilter may not return data on reverse scans (a…
b7748ff
…pache#6457)

Navigation Menu

Uh oh!

HBASE-28634 Fix FuzzyRowFilter may not return data on reverse scans - #6457

HBASE-28634 Fix FuzzyRowFilter may not return data on reverse scans#6457
stoty merged 9 commits into
apache:masterfrom
PDavid:HBASE-28634-FuzzyRowFilter-reverse-no-data

PDavid commented Nov 11, 2024•
edited
Loading

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

PDavid commented Nov 13, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

stoty left a comment

Uh oh!

Uh oh!

stotyNov 13, 2024

Uh oh!

PDavidNov 18, 2024

Uh oh!

stoty commented Nov 18, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

Apache-HBase commented Nov 20, 2024

Uh oh!

PDavid commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

PDavid commented Nov 11, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

PDavid commented Nov 13, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stotyNov 13, 2024

Choose a reason for hiding this comment

Uh oh!

PDavidNov 18, 2024

Choose a reason for hiding this comment

Uh oh!

stoty commented Nov 18, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

Apache-HBase commented Nov 20, 2024

Uh oh!

PDavid commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PDavid commented Nov 11, 2024•
edited
Loading