HBASE-28627 REST ScannerModel doesn't support includeStartRow/includeStopRow(addendum) by chandrasekhar-188k · Pull Request #6499 · apache/hbase

chandrasekhar-188k · 2024-11-28T16:00:41Z

HBASE-28627 REST ScannerModel doesn't support includeStartRow/includeStopRow(addendum)

stoty · 2024-12-02T06:06:22Z

 this.includeStartRow = includeStartRow;
 }

+ private static class IncludeStartRowFIlter {


Typo, should be IncludeStartRowFilter

stoty · 2024-12-02T06:07:43Z

 private boolean cacheBlocks = true;
 private int limit = -1;

+ @JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = IncludeStartRowFIlter.class)


Why do we need the filter for this and not for the stopRow ?
Wouldn't JsonInclude.Include.NON_DEFAULT work here the same ?

Also, a Unit test to check this serialization behaviour would be helpful.

To make sure that it survives any later refactors, etc.

Looks like there is already a test, it just needs to be updated.

Why do we need the filter for this and not for the stopRow ?
JsonInclude.Include.NON_DEFAULT --> this annotation serialize the attribute only if the attribute value is different from it's default value , ex: for boolean variables if the value is false(default) it will not get serialized, if the value is true it gets serialized.
Since includeStopRow is false by default we can use this annotation, where as includeStartRow is true by default, so we can't use this annotation because when the value is set to false this attribute will not get serialized and it will be giving wrong results in the response of scan request.

Looks like there is already a test, it just needs to be updated.
updated

OK, I get it. I thought that the default referred to the enclosing type, not to the boolean primitive.

stoty

Please see my comments

chandrasekhar-188k · 2024-12-15T08:23:01Z

Please see my comments

review comments are fixed, pls check

Apache-HBase · 2024-12-25T13:23:00Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 43s		Docker mode activated.
-0 ⚠️	yetus	0m 4s		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	2m 44s		master passed
+1 💚	compile	0m 23s		master passed
+1 💚	javadoc	0m 18s		master passed
+1 💚	shadedjars	5m 11s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	2m 43s		the patch passed
+1 💚	compile	0m 22s		the patch passed
+1 💚	javac	0m 22s		the patch passed
+1 💚	javadoc	0m 17s		the patch passed
+1 💚	shadedjars	4m 55s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	3m 13s		hbase-rest in the patch passed.
		21m 52s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6499/5/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#6499
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux 53a63a8ffb9b 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 / `074f161`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6499/5/testReport/
Max. process+thread count	2082 (vs. ulimit of 30000)
modules	C: hbase-rest U: hbase-rest
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6499/5/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.

Apache-HBase · 2024-12-25T13:27:27Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 42s		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	2m 52s		master passed
+1 💚	compile	0m 34s		master passed
+1 💚	checkstyle	0m 11s		master passed
+1 💚	spotbugs	0m 38s		master passed
+1 💚	spotless	0m 44s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+1 💚	mvninstall	2m 40s		the patch passed
+1 💚	compile	0m 33s		the patch passed
+1 💚	javac	0m 33s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 11s		the patch passed
+1 💚	spotbugs	0m 40s		the patch passed
+1 💚	hadoopcheck	9m 39s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 40s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 11s		The patch does not generate ASF License warnings.
		26m 20s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6499/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6499
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux ceef54b14c8b 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 / `074f161`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	85 (vs. ulimit of 30000)
modules	C: hbase-rest U: hbase-rest
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6499/5/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.

chandrasekhar-188k · 2025-01-09T15:25:15Z

Please see my comments
review comments are fixed, pls check

@stoty review comments are addressed and the build is okay, please review the changes again

stoty

Basically fine, but could be more readable, please see my comments.

stoty · 2025-01-09T17:24:07Z

+ private static class IncludeStartRowFilter {
+ @Override
+ public boolean equals(Object value) {
+ return Boolean.TRUE.equals(value);


Since this code is not performance critial, we could just check for the type and avoid the suppression:
if(value instanceOf Boolean) { return Boolean.TRUE.equals(value); } else { return false; }

according to the documents EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS checks if we are doing an instanceof check on any thing other than the current class. in this case it is IncludeStartRowFilter.
the equals() implementation of Boolean class as below.

Since it is trying to check instanceof Boolean which is not in the Hierachy of IncludeStartRowFilter find bugs is reporting it as voilation.
I had tried your suggestion before, but it still fails due to above reason.

Don't you love static checkers ?
OK, let's leave it like this.

stoty · 2025-01-09T17:29:02Z

+ return Boolean.TRUE.equals(value);
+ }
+
+ @Override


This is confusing, and I don't think this is necessary.
hashCode() is never called for for this object according to the docs.

This is confusing, and I don't think this is necessary.
hashCode() is never called for for this object according to the docs.
hashCode is never called, I have added this dummy hashCode implementation as the spotbugs was complaining about the rule:

we can either supress the check or add a dummy hashCode implementation.

In that case add the suppression instead.

it's less bad.

PR is updated with this change

…StopRow (addendum)

stoty

+1 LGTM

Thank you

…StopRow (addendum) (apache#6499) Signed-off-by: Istvan Toth <stoty@apache.org>

chandrasekhar-188k mentioned this pull request Nov 28, 2024
HBASE-28627 REST ScannerModel doesn't support includeStartRow/include… #6374
Merged

stoty reviewed Dec 2, 2024
View reviewed changes

stoty suggested changes Dec 2, 2024
View reviewed changes

This comment has been minimized.
Sign in to view

chandrasekhar-188k force-pushed the br_HBASE-28627_fix branch from cd1d831 to 58f8494Compare December 15, 2024 08:21

chandrasekhar-188k closed this Dec 15, 2024

chandrasekhar-188k reopened this Dec 15, 2024

chandrasekhar-188k requested a review from stoty December 15, 2024 08:26

chandrasekhar-188k force-pushed the br_HBASE-28627_fix branch from 58f8494 to d7935baCompare December 15, 2024 08:46

This comment has been minimized.
Sign in to view

chandrasekhar-188k force-pushed the br_HBASE-28627_fix branch from d7935ba to 074f161Compare December 25, 2024 12:48

stoty suggested changes Jan 9, 2025
View reviewed changes

HBASE-28627 REST ScannerModel doesn't support includeStartRow/include…
db05432
…StopRow (addendum)

chandrasekhar-188k force-pushed the br_HBASE-28627_fix branch from 074f161 to db05432Compare January 10, 2025 14:53

stoty approved these changes Jan 10, 2025
View reviewed changes

stoty merged commit 7134f29 into apache:masterJan 10, 2025

ragarkar pushed a commit to ragarkar/hbase that referenced this pull request Jan 13, 2025
HBASE-28627 REST ScannerModel doesn't support includeStartRow/include…
65b005c
…StopRow (addendum) (apache#6499) Signed-off-by: Istvan Toth <stoty@apache.org>

stoty mentioned this pull request Feb 12, 2025
HBASE-29125 Suppress HE_EQUALS_USE_HASHCODE in org.apache.hadoop.hbas… #6685
Merged

Uh oh!

Conversation

chandrasekhar-188k commented Nov 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

chandrasekhar-188k commented Dec 15, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Apache-HBase commented Dec 25, 2024

Uh oh!

Apache-HBase commented Dec 25, 2024

Uh oh!

chandrasekhar-188k commented Jan 9, 2025

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

stotyJan 9, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chandrasekhar-188kJan 10, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chandrasekhar-188kJan 10, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stotyJan 9, 2025•
edited
Loading

chandrasekhar-188kJan 10, 2025•
edited
Loading

chandrasekhar-188kJan 10, 2025•
edited
Loading