HBASE-29325 Gson reflection failures on TestBucketCache.testCacheSimple #6999

wchevreuil · 2025-05-19T11:53:24Z

No description provided.

Change-Id: I2a2365df7c72d3f9f26175d43904375888742eb8

stoty · 2025-05-19T12:52:04Z

The PR that has added this patch seems to expect that transient is excluded by default, as it added transient to several members.

TBH I'm not sure what we use the JSON serialization for. Is that only for logging/debugging ?

wchevreuil · 2025-05-21T14:48:41Z

The PR that has added this patch seems to expect that transient is expected by default, as it added transient to several members.

TBH I'm not sure what we use the JSON serialization for. Is that only for logging/debugging ?

I think transient was added as an attempt to get those fields ignored? But for some reason, GSON doesn't ignore it unless explicitly told to.

Apache9 · 2025-05-21T15:05:11Z

+1 with stoty, is this more like a test problem or an actual problem in main code?

Change-Id: I74e1e760949798a23755fdc959b51d4ede0affa1

wchevreuil · 2025-05-22T10:07:19Z

+1 with stoty, is this more like a test problem or an actual problem in main code?

I have seen this manifesting on tests using specific JDK 17 implementations ("Java HotSpot(TM) 64-Bit Server VM (build 17.0.8+9-LTS-211)" and "OpenJDK Runtime Environment (build 17.0.15+6-Ubuntu-0ubuntu124.04)"). Apparently, "Eclipse Adoptium-17.0.11+9" works just fine, as we don't seen this in neither the precommits nor in nightly builds. I guess this error would blow up in any part of the code that tries to convert internal objects to json. I think this is mainly used on the UIs and logging.

stoty · 2025-05-22T11:06:26Z

hbase-common/src/main/java/org/apache/hadoop/hbase/util/GsonUtil.java

@@ -43,6 +44,8 @@ private GsonUtil() {
   */
  public static GsonBuilder createGson() {
    return new GsonBuilder().setLongSerializationPolicy(LongSerializationPolicy.STRING)
+      .excludeFieldsWithModifiers(Modifier.TRANSIENT).excludeFieldsWithModifiers(Modifier.STATIC)


Do we need to exclude static and private too?

I think STATIC and TRANSIENT are default exclusions, so if we are explicitly adding TRANSIENT, we may have to do STATIC as well.
Do we need to do for PRIVATE?

Do we need to exclude static and private too?

We need only private, actually. I'm updating the PR accordingly.

Ah, I see it now. You're right.

Change-Id: Ic7cf980db55c5ac7b6551f7eab44cf04fb7f51d2

Change-Id: Id67614680b8bc1df1fdb4e6aa2cb63c9798ded9e

stoty

+1 LGTM

The JSON serialization (at least BlockCacheUtil.toJSON) is only used for logging from tests.

Apache9 · 2025-05-28T14:39:37Z

We do not want to serialize private fields? Really? I think most POJOs are private fields with getter and setter?

stoty · 2025-05-28T15:46:19Z

We do not want to serialize private fields? Really? I think most POJOs are private fields with getter and setter?

The JSON serialization is used in a single test in debug log output.

If/when this will be needed again, the person doing the dbugging can figure out which fields they need and fix the JSON serialization.
At least the test suite runs on the latest JDK with these changes.

Kota-SH

LGTM

Apache-HBase · 2025-05-28T18:04:27Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 27s		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 19s		master passed
+1 💚	compile	0m 19s		master passed
+1 💚	javadoc	0m 17s		master passed
+1 💚	shadedjars	6m 6s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	3m 9s		the patch passed
+1 💚	compile	0m 19s		the patch passed
+1 💚	javac	0m 19s		the patch passed
+1 💚	javadoc	0m 15s		the patch passed
+1 💚	shadedjars	6m 4s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	2m 39s		hbase-common in the patch passed.
		23m 57s

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

Apache-HBase · 2025-05-28T18:12:04Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 36s		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	3m 20s		master passed
+1 💚	compile	0m 41s		master passed
+1 💚	checkstyle	0m 14s		master passed
+1 💚	spotbugs	0m 32s		master passed
+1 💚	spotless	0m 50s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+1 💚	mvninstall	3m 10s		the patch passed
+1 💚	compile	0m 40s		the patch passed
+1 💚	javac	0m 40s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 14s		the patch passed
+1 💚	spotbugs	0m 38s		the patch passed
+1 💚	hadoopcheck	12m 7s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 45s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 10s		The patch does not generate ASF License warnings.
		31m 34s

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

stoty · 2025-05-29T05:02:26Z

The JSON serialization is used in a single test in debug log output.

No it's not, we just got another ticket that demonstrates that it's also called by the UI from JSP.

stoty

GSONUtil is also used from other classes.
I misclicked when checking for references.

wchevreuil · 2025-05-29T10:19:43Z

The JSON serialization is used in a single test in debug log output.

No it's not, we just got another ticket that demonstrates that it's also called by the UI from JSP.

Yeah, that's a problem. Imagine a file based bucket cache of TBs of data, the in-memory representation of the cache structure can easily grow to few GBs of RAM.

Parsing it into a String can cause potential problems for the RS.

I wonder if we should rather remove JSON deserialisation for BucketCache. Or at least put some safeguards that check for the size of the BucketCache object.

Also, ain't sure how useful such a large string would be.

stoty · 2025-05-29T11:01:32Z

Yes, using GSON to serializalize BucketCache is a clusterfuck.

We need to check if if / how much if it the UI needs, and find some solution.

stoty · 2025-05-29T11:11:41Z

I wonder if we should rather remove JSON deserialisation for BucketCache. Or at least put some safeguards that check for the size of the BucketCache object.

Yes, it's completely bonkers.
On closer look, HBASE-29362 is not using the BucketCache object itself, so it's not really realted to this issue.

I totally agree that we should just remove all code that tries to use Gson to serialize complext multigigabyte in-memory cache objects with reflection.

TBH Gson itself looks like a spectaculrarly bad fit with modular JAva, and real objects.
Maybe it's OK for simle POJO DTOs.

stoty · 2025-05-29T11:21:26Z

No it's not, we just got another ticket that demonstrates that it's also called by the UI from JSP.

We're calling GsonUtil from rits.jsp , but luckily that's creating a simple map for the values needed on the UI, and not doing anything stupid like trying to serialize BucketCache.

wchevreuil · 2025-05-29T11:25:41Z

No it's not, we just got another ticket that demonstrates that it's also called by the UI from JSP.

We're calling GsonUtil from rits.jsp , but luckily that's creating a simple map for the values needed on the UI, and not doing anything stupid like trying to serialize BucketCache.

There seems to be an option where we serialize the entire cache in the UI: https://github.com/apache/hbase/pull/7047/files#diff-a0cc60aec48986f0ec2377528ab6c26f248c246267b57145a1bd3ab9c7e42d07R60

stoty · 2025-05-29T11:32:44Z

You're right, we're doing that.
Thanks for catching @wchevreuil .

HBASE-29325 Gson reflection failures on TestBucketCache.testCacheSimple

27c6ca7

Change-Id: I2a2365df7c72d3f9f26175d43904375888742eb8

This comment has been minimized.

Sign in to view

spotless

fd495a1

Change-Id: I74e1e760949798a23755fdc959b51d4ede0affa1

stoty reviewed May 22, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

wchevreuil added 2 commits May 28, 2025 10:57

removing filtering of static and transient

22f492e

Change-Id: Ic7cf980db55c5ac7b6551f7eab44cf04fb7f51d2

removing commented code

bbbf567

Change-Id: Id67614680b8bc1df1fdb4e6aa2cb63c9798ded9e

stoty approved these changes May 28, 2025

View reviewed changes

Kota-SH approved these changes May 28, 2025

View reviewed changes

stoty requested changes May 29, 2025

View reviewed changes

HBASE-29325 Gson reflection failures on TestBucketCache.testCacheSimple #6999

Are you sure you want to change the base?

HBASE-29325 Gson reflection failures on TestBucketCache.testCacheSimple #6999

Conversation

wchevreuil commented May 19, 2025

Uh oh!

stoty commented May 19, 2025 • 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.

wchevreuil commented May 21, 2025

Uh oh!

Apache9 commented May 21, 2025

Uh oh!

wchevreuil commented May 22, 2025

Uh oh!

stoty May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Kota-SH May 27, 2025

Choose a reason for hiding this comment

Uh oh!

wchevreuil May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Kota-SH May 28, 2025

Choose a reason for hiding this comment

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!

Apache9 commented May 28, 2025

Uh oh!

stoty commented May 28, 2025

Uh oh!

Kota-SH left a comment

Choose a reason for hiding this comment

Uh oh!

Apache-HBase commented May 28, 2025

Uh oh!

Apache-HBase commented May 28, 2025

Uh oh!

stoty commented May 29, 2025

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

wchevreuil commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoty commented May 29, 2025

Uh oh!

stoty commented May 29, 2025

Uh oh!

wchevreuil commented May 29, 2025

Uh oh!

stoty commented May 29, 2025

Uh oh!

Uh oh!

stoty commented May 19, 2025 •

edited

Loading

wchevreuil commented May 29, 2025 •

edited

Loading

stoty commented May 29, 2025 •

edited

Loading