HBASE-29742 Compaction scan returns single cells instead of rows after 10MB

[stoty]

No description provided.

[Apache9]

The naming and logic is a bit confusing here, we have a keepProgress field here, but we use it outside ScannerContext to determine whether we should call clearProgress, but for the keepBlockProgress, we use it inside the ScannerContext while calling the clearProgress method...

[Apache9]

And I can not recall why block size should not be cleared...

Need to do git blame...

[Apache9]

Seems to be broken by HBASE-27558, where we introduce quotas for total block io for scans.

[Apache-HBase]

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	2m 10s		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 _
+1 💚	mvninstall	5m 38s		master passed
+1 💚	compile	4m 25s		master passed
+1 💚	checkstyle	1m 18s		master passed
+1 💚	spotbugs	2m 14s		master passed
-1 ❌	spotless	0m 15s		branch has 60 errors when running spotless:check, run spotless:apply to fix.
			_ Patch Compile Tests _
+1 💚	mvninstall	5m 5s		the patch passed
+1 💚	compile	4m 32s		the patch passed
+1 💚	javac	4m 32s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 16s		the patch passed
+1 💚	spotbugs	2m 23s		the patch passed
+1 💚	hadoopcheck	15m 38s		Patch does not cause any errors with Hadoop 3.3.6 3.4.1.
-1 ❌	spotless	0m 13s		patch has 60 errors when running spotless:check, run spotless:apply to fix.
			_ Other Tests _
+1 💚	asflicense	0m 16s		The patch does not generate ASF License warnings.
		55m 10s

Subsystem	Report/Notes
Docker	ClientAPI=1.48 ServerAPI=1.48 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7517/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#7517
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux 5c50491824e2 6.8.0-1024-aws #26~22.04.1-Ubuntu SMP Wed Feb 19 06:54:57 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / 02d8131
Default Java	Eclipse Adoptium-17.0.11+9
spotless	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7517/1/artifact/yetus-general-check/output/branch-spotless.txt
spotless	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7517/1/artifact/yetus-general-check/output/patch-spotless.txt
Max. process+thread count	111 (vs. ulimit of 30000)
modules	C: hbase-server U: hbase-server
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7517/1/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.

[stoty]

The naming and logic is a bit confusing here, we have a keepProgress field here, but we use it outside ScannerContext to determine whether we should call clearProgress, but for the keepBlockProgress, we use it inside the ScannerContext while calling the clearProgress method...

Yes, this is a sub-case of the keepProgress=false case.
I don't think pushing this logic to the StoreScanner would improve readability.

I'm of course open to suggestions on changing names or implementation.

[stoty]

And I can not recall why block size should not be cleared...

Need to do git blame...

TBH that's not clear to me either.

[Apache-HBase]

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 33s		Docker mode activated.
-0 ⚠️	yetus	0m 3s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ master Compile Tests _
+1 💚	mvninstall	3m 51s		master passed
+1 💚	compile	1m 2s		master passed
+1 💚	javadoc	0m 33s		master passed
+1 💚	shadedjars	6m 16s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	3m 17s		the patch passed
+1 💚	compile	1m 2s		the patch passed
+1 💚	javac	1m 2s		the patch passed
+1 💚	javadoc	0m 31s		the patch passed
+1 💚	shadedjars	6m 15s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	209m 51s		hbase-server in the patch passed.
		237m 25s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7517/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#7517
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux f7b8fdfb9a3a 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / 02d8131
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7517/1/testReport/
Max. process+thread count	4550 (vs. ulimit of 30000)
modules	C: hbase-server U: hbase-server
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7517/1/console
versions	git=2.34.1 maven=3.9.8
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

[wchevreuil]

LGTM, just please address spotless issues.

[Apache9]

The spotless issues will be fixed by #7507.

I think we'd better try to understand the code added in HBASE_27558, and see what is the correct way to fix this, instead of just adding a side logic to bypass the check...

[stoty]

The spotless issues will be fixed by #7507.

I think we'd better try to understand the code added in HBASE_27558, and see what is the correct way to fix this, instead of just adding a side logic to bypass the check...

Reading the text of HBASE-27558 , it's not reset because the block limit was meant for quota enforcement, not for limiting the RPC response time / size, like the rest of the limits.

I agree that this is not the cleanest design, but ScannerContext is the mechanism for we have for tracking resource usage in StoreScanner and returning early, and adding a separate mechanism for quota enforcement would not necessarily be an improvement.

We could perhaps generalize the behaviour to be per limit to make it more explicit.

Today we are already treating the time limit differently, as it can not be reset by implementation,
the batch limit is always checked between cells, and blockSize is never reset.
so there are many implicit behavioural differences between how the limits are handled.

Do you think making these implicit per-limit options explicitly configurable for each limit help ? I'm not sure.
Maybe we should just add more comments to clarify behaviour ?

@bbeaudreault what's your take ?

[Apache9]

Checked the code, the only place where we set keepProgress to true is in RegionScannerImpl.populateResult, where we want to keep progress across families, i.e, different StoreScanners. This is the keepProgress designed to do.

The ScannerContext is not designed to enforce quota, so I think the usage in HBASE-27558 is incorrect, at least, it should not mess up with other limits.

Let me think what is the better way to fix it, at least, we should change the code added in HBASE-27558, not the code in other places...

Thanks.