HBASE-29706: Alter table with reopen false should pass if coprocessors havenot changed #7575

mnpoonia · 2025-12-24T15:38:21Z

No description provided.

Umeshkumar9414 · 2025-12-24T18:40:27Z

Need to run mvn spotless:apply

...-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java

Umeshkumar9414

LGTM

mnpoonia · 2025-12-26T15:48:01Z

@apurtell @virajjasani please review when you get some free cycle.

Apache9

The title of the PR is different with the title of the jira...

Could someone explain more about what do we actually fix here?

Thanks.

Apache9 · 2025-12-27T12:09:07Z

hbase-client/src/main/java/org/apache/hadoop/hbase/client/CoprocessorDescriptorBuilder.java

+      ) {
+        return false;
+      }
+      return new TreeMap<>(getProperties()).equals(new TreeMap<>(other.getProperties()));


I think we could use equals directly here?

Fixed and improved it.

Apache9 · 2025-12-27T12:10:24Z

hbase-client/src/main/java/org/apache/hadoop/hbase/client/CoprocessorDescriptorBuilder.java

+
+    @Override
+    public int hashCode() {
+      return Objects.hash(className, jarPath, priority, new TreeMap<>(properties));


Better use Objects.hash to calculate hashCode for fields other than properties, and then use stream.sorted to iterator over properties and calculate hashCode on the fly.

Done. Almost what you suggested.

Apache9 · 2025-12-27T12:15:36Z

hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java

      if (
-        this.unmodifiedTableDescriptor.getCoprocessorDescriptors().hashCode()
-            != this.modifiedTableDescriptor.getCoprocessorDescriptors().hashCode()
+        !new HashSet<>(this.unmodifiedTableDescriptor.getCoprocessorDescriptors())


This is not on the critial read/write path so maybe this is acceptable, but a better way, is to first check size, and then, create a HashSet for one of the descriptors, and then iterator over the other one, and check whethere the HashSet contains it.

…s havenot changed

Apache9 · 2025-12-29T10:35:02Z

hbase-client/src/main/java/org/apache/hadoop/hbase/client/CoprocessorDescriptorBuilder.java

+    @Override
+    public int hashCode() {
+      int result = Objects.hash(className, jarPath, priority);
+      List<Map.Entry<String, String>> entries = new ArrayList<>(properties.entrySet());


Ah, checked the code, properties is already a TreeMap... So we do not need to sort here, just iterate over it is enough...

To make it safe, we can change the declaration of properties from Map to SortedMap or NavigableMap.

Consdering this change is not in hot path should we still change the treemap to map(Navigable or sortable)?
This might become a cleanup exercise where treemap specific functionality is used elsewhere. I mean i am happy to change it but doesn't make sense to do it in this jira atleast.

Let me know if you want it within same jira.

I think it is related. We want to make sure the hash code calculation is stable, so here we want to sort the entries before calculating. Since we already use TreeMap in CoprocessorDescriptorBuilder, we can make sure that the Map in CoprocessorDescriptor is a TreeMap, which is sorted, so we do not need to sort it again.

To prevent later developers break the assumption, i.e, changing the TreeMap to HashMap in CoprocessorDescriptorBuilder without any compilation errors, we'd better change the declaration in CoprocessorDescriptor to SortedMap or NavigableMap, so the constructor will not accept a HashMap any more. And we could also add some comments to explain that we must use SortedMap here as we need it for a stable hash code calculation.

Thanks.

Thanks @Apache9 . I think after we move to NavigableMap we can change the whole hashCode() method to a simple Objects.hash(className, jarPath, priority, properties);

This will be exactly equivalent to current logic.

Updated the PR. Have updated the Map to NavigableMap in interface as well. Let me know what you think about the same.

@Apache9 if you think we should still calculate hash for properties on the fly and use object hash for others let me know. I think current logic of just using objects.hash should suffice here after our change in equals logic and change of properties to navigablemap

…dingly

Apache-HBase · 2025-12-30T05:25:12Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 39s		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 38s		Maven dependency ordering for branch
+1 💚	mvninstall	4m 13s		master passed
+1 💚	compile	5m 11s		master passed
+1 💚	checkstyle	1m 36s		master passed
+1 💚	spotbugs	3m 4s		master passed
+1 💚	spotless	0m 57s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 41s		the patch passed
+1 💚	compile	5m 5s		the patch passed
+1 💚	javac	5m 5s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 36s		the patch passed
+1 💚	spotbugs	3m 12s		the patch passed
+1 💚	hadoopcheck	14m 4s		Patch does not cause any errors with Hadoop 3.3.6 3.4.1.
+1 💚	spotless	0m 52s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 21s		The patch does not generate ASF License warnings.
		54m 1s

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

Apache-HBase · 2025-12-30T08:34:56Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 30s		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 _
+0 🆗	mvndep	0m 34s		Maven dependency ordering for branch
+1 💚	mvninstall	3m 34s		master passed
+1 💚	compile	1m 22s		master passed
+1 💚	javadoc	0m 45s		master passed
+1 💚	shadedjars	6m 15s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 7s		the patch passed
+1 💚	compile	1m 22s		the patch passed
+1 💚	javac	1m 22s		the patch passed
+1 💚	javadoc	0m 45s		the patch passed
+1 💚	shadedjars	6m 14s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	1m 32s		hbase-client in the patch passed.
+1 💚	unit	212m 36s		hbase-server in the patch passed.
		243m 55s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7575/5/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#7575
JIRA Issue	HBASE-29706
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux 54ad26445290 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 / `300fb24`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7575/5/testReport/
Max. process+thread count	4557 (vs. ulimit of 30000)
modules	C: hbase-client hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7575/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.

mnpoonia force-pushed the hbase_lazy_mode branch from cd74717 to 4272f2c Compare December 24, 2025 15:44

This comment has been minimized.

Sign in to view

Umeshkumar9414 reviewed Dec 26, 2025

View reviewed changes

...-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestModifyTableProcedure.java Outdated Show resolved Hide resolved

Umeshkumar9414 approved these changes Dec 26, 2025

View reviewed changes

mnpoonia force-pushed the hbase_lazy_mode branch from 4272f2c to 3a1ebef Compare December 26, 2025 06:09

This comment has been minimized.

Sign in to view

Apache9 reviewed Dec 27, 2025

View reviewed changes

HBASE-29706: Alter table with reopen false should pass if coprocessor…

d66e3ac

…s havenot changed

mnpoonia force-pushed the hbase_lazy_mode branch from 3a1ebef to d66e3ac Compare December 29, 2025 09:44

mnpoonia changed the title ~~HBASE-29706: Make CoprocessorDescriptorImpl.equals() and hashCode() r…~~ HBASE-29706: Alter table with reopen false should pass if coprocessors havenot changed Dec 29, 2025

This comment has been minimized.

Sign in to view

Apache9 reviewed Dec 29, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Change map to navigable map and optimize the object hash method accor…

300fb24

…dingly

HBASE-29706: Alter table with reopen false should pass if coprocessors havenot changed #7575

Are you sure you want to change the base?

HBASE-29706: Alter table with reopen false should pass if coprocessors havenot changed #7575

Conversation

mnpoonia commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Umeshkumar9414 commented Dec 24, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Umeshkumar9414 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

mnpoonia commented Dec 26, 2025

Uh oh!

Apache9 left a comment

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!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

mnpoonia Dec 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Apache-HBase commented Dec 30, 2025

Uh oh!

Apache-HBase commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mnpoonia commented Dec 24, 2025 •

edited

Loading

mnpoonia Dec 29, 2025 •

edited

Loading