HBASE-28627 REST ScannerModel doesn't support includeStartRow/include… by chandrasekhar-188k · Pull Request #6374 · apache/hbase

chandrasekhar-188k · 2024-10-20T12:07:19Z

…StopRow

stoty · 2024-10-22T14:32:40Z

Thank you.

The thorny issue here backwards-forward compatibility.

i.e. What happens if a client without this patch tries to run a scan on server with this change, and vice versa, with all three (json,xml,protobuf) encodings.

Unfortnuately, the easiest way to test that is bringing up an old/new cluster (can be a single process one), and trying a scan with the new/old client manually for all six cases.

To be clear, it's fine for the old server to fail when the new parameters are set, but it should work if they are not set, or are at the default value.

chandrasekhar-188k · 2024-11-07T10:05:18Z

Thank you.
The thorny issue here backwards-forward compatibility.
i.e. What happens if a client without this patch tries to run a scan on server with this change, and vice versa, with all three (json,xml,protobuf) encodings.
Unfortnuately, the easiest way to test that is bringing up an old/new cluster (can be a single process one), and trying a scan with the new/old client manually for all six cases.
To be clear, it's fine for the old server to fail when the new parameters are set, but it should work if they are not set, or are at the default value.

i have tested for JSON with new server and new/old clients, i will test for other combinations also and update the test results.

chandrasekhar-188k · 2024-11-10T01:41:14Z

i.e. What happens if a client without this patch tries to run a scan on server with this change, and vice versa, with all three (json,xml,protobuf) encodings.

I have tested for all the combinations and results are fine.

REST_Scan.ods

…StopRow

Apache-HBase · 2024-11-11T13:26:22Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 43s		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.
+0 🆗	buf	0m 0s		buf was not available.
+0 🆗	buf	0m 0s		buf 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	3m 16s		master passed
+1 💚	compile	0m 45s		master passed
+1 💚	checkstyle	0m 16s		master passed
+1 💚	spotbugs	0m 57s		master passed
+1 💚	spotless	1m 5s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+1 💚	mvninstall	3m 34s		the patch passed
+1 💚	compile	0m 43s		the patch passed
+1 💚	cc	0m 43s		the patch passed
-0 ⚠️	javac	0m 43s	/results-compile-javac-hbase-rest.txt	hbase-rest generated 2 new + 183 unchanged - 0 fixed = 185 total (was 183)
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 12s		the patch passed
+1 💚	spotbugs	0m 54s		the patch passed
+1 💚	hadoopcheck	12m 22s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	hbaseprotoc	0m 26s		the patch passed
+1 💚	spotless	0m 54s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 11s		The patch does not generate ASF License warnings.
		33m 58s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6374/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6374
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless cc buflint bufcompat hbaseprotoc
uname	Linux 4c1f57507408 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 / `2d3a36d`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	84 (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-6374/3/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.

Apache-HBase · 2024-11-11T13:37:33Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 41s		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 2s		master passed
+1 💚	compile	0m 23s		master passed
+1 💚	javadoc	0m 19s		master passed
+1 💚	shadedjars	5m 22s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	2m 52s		the patch passed
+1 💚	compile	0m 24s		the patch passed
+1 💚	javac	0m 24s		the patch passed
+1 💚	javadoc	0m 18s		the patch passed
+1 💚	shadedjars	5m 13s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	3m 39s		hbase-rest in the patch passed.
		23m 18s

Subsystem	Report/Notes
Docker	ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6374/3/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#6374
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux 9ea8555c21e5 5.4.0-195-generic #215-Ubuntu SMP Fri Aug 2 18:28:05 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `2d3a36d`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6374/3/testReport/
Max. process+thread count	1542 (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-6374/3/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.

stoty · 2024-11-25T09:39:16Z

Thanks.

Can you please do a backport PR for branch-2 @chandrasekhar-188k ?

chandrasekhar-188k · 2024-11-26T14:05:19Z

Thanks.
Can you please do a backport PR for branch-2 @chandrasekhar-188k ?

okay

chandrasekhar-188k · 2024-11-26T14:40:42Z

Thanks.
Can you please do a backport PR for branch-2 @chandrasekhar-188k ?

backport PR raised for branch-2, please review
#6494

stoty

+1 LGTM

stoty · 2024-11-27T10:02:06Z

After commiting this (a little late), I realized that I have more questions:

I see this annotation on ScannerModel:

@JsonInclude(JsonInclude.Include.NON_NULL)

and

private boolean includeStartRow = true;
private boolean includeStopRow = false;

Doesn't that mean that the new attributes will get serialized regardless of their values to JSON,
and cause the deserialization in the old server to fail ?

You matrix shows this as passing, but I can't see how that can happen.
Does JSON skip serializing the default values ?

chandrasekhar-188k · 2024-11-27T18:31:04Z

After commiting this (a little late), I realized that I have more questions:
I see this annotation on ScannerModel:
@JsonInclude(JsonInclude.Include.NON_NULL)
and
private boolean includeStartRow = true;
private boolean includeStopRow = false;
Doesn't that mean that the new attributes will get serialized regardless of their values to JSON, and cause the deserialization in the old server to fail ?
You matrix shows this as passing, but I can't see how that can happen. Does JSON skip serializing the default values ?

you are correct, when we serialize the ScannerModel the new attributes are also included and causing the deserilization to fail at old server.
when I tested for JSON I have construcuted the JSON message manually with the required fields, instead of Converting the ScannerModel to JSON, that's why my test was ok.
For XML and protobuff I had tested with Conversion of ScannerModel object to the required mime type, so no problem here.

For solving the issue wrt JSON, I think we can have two approaches

change the class level JSON include annotation to @JsonInclude(JsonInclude.Include.NON_DEFAULT), so that it doesn't serialize the default values. This can work with out any functional impact only if the default values of ScannerModel and Scan Class are in sync always.. but currently I could see the default values for some fields are different between ScannerModel and Scan.
For the newly introduced attributes add a custom JSON include annotation with a filter class that skips serialization of a field if the field has default value set to it.
ex:

@JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = MaskingFilter.class)
private boolean includeStartRow = true;
static class MaskingFilter {
public boolean equals(Object value){
return Boolean.TRUE.equals(value);
}
}

I think the 2nd approach is better for this. I will raise an addendum PR for this..Let me know if you have any other suggestions..

…StopRow (#6374) Signed-off-by: Istvan Toth <stoty@apache.org> (cherry picked from commit f9ec1dc)

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

After commiting this (a little late), I realized that I have more questions:
I see this annotation on ScannerModel:
@JsonInclude(JsonInclude.Include.NON_NULL)
and
private boolean includeStartRow = true;
private boolean includeStopRow = false;
Doesn't that mean that the new attributes will get serialized regardless of their values to JSON, and cause the deserialization in the old server to fail ?
You matrix shows this as passing, but I can't see how that can happen. Does JSON skip serializing the default values ?
you are correct, when we serialize the ScannerModel the new attributes are also included and causing the deserilization to fail at old server. when I tested for JSON I have construcuted the JSON message manually with the required fields, instead of Converting the ScannerModel to JSON, that's why my test was ok. For XML and protobuff I had tested with Conversion of ScannerModel object to the required mime type, so no problem here.
For solving the issue wrt JSON, I think we can have two approaches
change the class level JSON include annotation to @JsonInclude(JsonInclude.Include.NON_DEFAULT), so that it doesn't serialize the default values. This can work with out any functional impact only if the default values of ScannerModel and Scan Class are in sync always.. but currently I could see the default values for some fields are different between ScannerModel and Scan.
For the newly introduced attributes add a custom JSON include annotation with a filter class that skips serialization of a field if the field has default value set to it.
ex:
@JsonInclude(value = JsonInclude.Include.CUSTOM, valueFilter = MaskingFilter.class)
private boolean includeStartRow = true;
static class MaskingFilter {
public boolean equals(Object value){
return Boolean.TRUE.equals(value);
}
}
I think the 2nd approach is better for this. I will raise an addendum PR for this..Let me know if you have any other suggestions..

addendum PR raised for this fix, Please review
#6499

PDavid · 2024-12-05T09:00:13Z

+ @DefaultValue("true") @QueryParam(Constants.SCAN_INCLUDE_START_ROW) boolean includeStartRow,
+ @DefaultValue("false") @QueryParam(Constants.SCAN_INCLUDE_STOP_ROW) boolean includeStopRow) {


I know that this is PR is already merged but maybe it would make sense to also log these new parameters in the trace log statement a bit below. Maybe this could be added in the followup PR: #6499
What do you think @chandrasekhar-188k ?

@PDavid ,
I have missed to check this.
#6499 is already merged. I will raise another PR for this change

stoty · 2024-12-06T07:51:35Z

Can you fix the test and answer my question question by mid next week @chandrasekhar-188k ?

I don't want to start the winter holidays with the unfixed compatibility issue, so if we cannot fix that, I will temporarily revert the patch until the compatibility issue can be fixed.

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

This comment has been minimized.
Sign in to view

chandrasekhar-188k force-pushed the br_HBASE-28627 branch from d6a342f to a2e0e1eCompare November 10, 2024 01:49

This comment has been minimized.
Sign in to view

HBASE-28627 REST ScannerModel doesn't support includeStartRow/include…
2d3a36d
…StopRow

chandrasekhar-188k force-pushed the br_HBASE-28627 branch from a2e0e1e to 2d3a36dCompare November 11, 2024 11:35

stoty approved these changes Nov 27, 2024
View reviewed changes

stoty merged commit f9ec1dc into apache:masterNov 27, 2024

stoty pushed a commit that referenced this pull request Nov 28, 2024
HBASE-28627REST ScannerModel doesn't support includeStartRow/include…
48df605
…StopRow (#6374) Signed-off-by: Istvan Toth <stoty@apache.org> (cherry picked from commit f9ec1dc)

PDavid reviewed Dec 5, 2024
View reviewed changes

gvprathyusha6 pushed a commit to gvprathyusha6/hbase that referenced this pull request Dec 19, 2024
HBASE-28627 REST ScannerModel doesn't support includeStartRow/include…
f923da5
…StopRow (apache#6374) Signed-off-by: Istvan Toth <stoty@apache.org>

		@DefaultValue("true") @QueryParam(Constants.SCAN_INCLUDE_START_ROW) boolean includeStartRow,
		@DefaultValue("false") @QueryParam(Constants.SCAN_INCLUDE_STOP_ROW) boolean includeStopRow) {

Uh oh!

Conversation

chandrasekhar-188k commented Oct 20, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

stoty commented Oct 22, 2024

Uh oh!

chandrasekhar-188k commented Nov 7, 2024

Uh oh!

chandrasekhar-188k commented Nov 10, 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.

Apache-HBase commented Nov 11, 2024

Uh oh!

Apache-HBase commented Nov 11, 2024

Uh oh!

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

Uh oh!

Uh oh!

chandrasekhar-188k commented Nov 26, 2024

Uh oh!

chandrasekhar-188k commented Nov 26, 2024

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

chandrasekhar-188k commented Nov 27, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chandrasekhar-188k commented Nov 28, 2024

Uh oh!

PDavidDec 5, 2024

Choose a reason for hiding this comment

Uh oh!

chandrasekhar-188kFeb 18, 2025

Choose a reason for hiding this comment

Uh oh!

stoty commented Dec 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chandrasekhar-188k commented Nov 10, 2024•
edited
Loading

stoty commented Nov 25, 2024•
edited
Loading

stoty commented Nov 27, 2024•
edited
Loading

chandrasekhar-188k commented Nov 27, 2024•
edited
Loading