HBASE-29026 Replace some deprecated calls #6585

PDavid · 2025-01-08T13:35:21Z

Replaced the following deprecated methods:
- java.net.URLEncoder.encode(String) -> java.net.URLEncoder.encode(String, Charset)
- org.apache.hadoop.util.StringUtils.humanReadableInt(long) -> org.apache.hadoop.util.StringUtils.TraditionalBinaryPrefix.long2String(long, "", 1): For this a new static util method is introduced: org.apache.hadoop.hbase.util.Strings.humanReadableInt
- org.apache.hadoop.fs.FileSystem.getLength(Path) -> getFileStatus(Path).getLen()
- org.apache.hadoop.hbase.ServerName.getStartcode() -> org.apache.hadoop.hbase.ServerName.getStartCode()
Also removed unused imports in the touched JSP files.

hbase-server/src/main/resources/hbase-webapps/master/procedures.jsp

PDavid · 2025-01-08T13:39:20Z

hbase-server/src/main/resources/hbase-webapps/master/table.jsp

        + master.getRegionServerInfoPort(addr) + "/rs-status";
  %>
      <tr>
-        <td><a href="<%= url %>"><%= StringEscapeUtils.escapeHtml4(addr.getHostname().toString())


A bit unrelated, but this toString() call was not required so removed it.

PDavid · 2025-01-09T07:41:16Z

In the PR build there were some failed tests (TestVerifyBucketCacheFile) but to me they look unrelated.

hbase-server/src/main/resources/hbase-webapps/master/procedures.jsp

hbase-server/src/main/resources/hbase-webapps/master/snapshot.jsp

hbase-server/src/main/resources/hbase-webapps/master/table.jsp

hbase-server/src/main/resources/hbase-webapps/master/snapshotsStats.jsp

NihalJain

replace org.apache.commons.lang3.StringEscapeUtils.escapeXml -> org.apache.commons.lang3.StringEscapeUtils.escapeXml10

PDavid · 2025-01-09T13:38:12Z

replace org.apache.commons.lang3.StringEscapeUtils.escapeXml -> org.apache.commons.lang3.StringEscapeUtils.escapeXml10

@NihalJain thanks for the review and the proposal. 👍 As I mentioned above - since the whole org.apache.commons.lang3.StringEscapeUtils class is deprecated I would rather replace it with the commons-text class.

NihalJain · 2025-01-09T13:41:49Z

replace org.apache.commons.lang3.StringEscapeUtils.escapeXml -> org.apache.commons.lang3.StringEscapeUtils.escapeXml10

@NihalJain thanks for the review and the proposal. 👍 As I mentioned above - since the whole org.apache.commons.lang3.StringEscapeUtils class is deprecated I would rather replace it with the commons-text class.

Oh ok, in that case we may need to add commons text as a dependency right? Until now we have used org.apache.commons.text* in only 2 test classes.

hbase-server/src/main/resources/hbase-webapps/regionserver/region.jsp

hbase-server/src/main/resources/hbase-webapps/master/rits.jsp

pom.xml

stoty · 2025-01-16T06:43:15Z

IMO HBase already has more than enough dependencies, we should try to avoid adding more if we can.

Is there a known security issue with the current xmlEscape ?
Is there something in another existing dependency that we could use ?

stoty · 2025-01-16T06:44:57Z

Even if it's already coming from Hadoop, it still bloats the Hadoop-less HBase assembly.

NihalJain · 2025-01-16T06:48:59Z

IMO HBase already has more than enough dependencies, we should try to avoid adding more if we can.

Is there a known security issue with the current xmlEscape ? Is there something in another existing dependency that we could use ?

+1 on idea of avoiding new dependency. I too am skeptical about this new addition and needed another set of eyes to approve of this. Thanks for chiming in @stoty !

stoty · 2025-01-17T09:17:28Z

I suggest reverting the change that requires the new commons-text dependency, and keeping the rest of the cleanups, @PDavid .

stoty · 2025-01-17T09:17:46Z

Unless it fixes a known security issue.

PDavid · 2025-01-20T09:42:13Z

Hi @NihalJain and @stotyű!

Many thanks for your feedback. 👍

My motivation of adding commons text was that the used API (org.apache.commons.lang3.StringEscapeUtils) was moved to commons text and it is marked as deprecated in commons lang. I think in a future version the deprecated API will be removed. That's why it is better to not depend on it in my opinion. Commons text is a small Apache library which does not have any other dependencies.

But of course if you wish, we can keep using the deprecated API as today it is still there.

stoty · 2025-01-20T09:45:53Z

Let's skip the commons-text dependency for now.

PDavid · 2025-01-20T10:40:28Z

hbase-server/src/main/resources/hbase-webapps/master/snapshotsStats.jsp

+  /**
+   * Note: This method was taken from org.apache.hadoop.util.StringUtils.humanReadableInt(long).
+   * Given an integer, return a string that is in an approximate, but human
+   * readable format.
+   * @param number the number to format
+   * @return a human readable form of the integer
+   */
+  private static String humanReadableInt(long number) {
+    return StringUtils.TraditionalBinaryPrefix.long2String(number, "", 1);
+  }


Should I maybe rather extract this method to a util class as it is now duplicated in these 2 JSP files?
Maybe it can be extracted to org.apache.hadoop.hbase.util.Strings util class.
What do you think?

Yes please. We have several instances of the deprecated method all over the code, we can re-use it for those cases as well. See https://github.com/search?q=repo%3Aapache%2Fhbase+StringUtils.humanReadableInt&type=code

Thanks, this should be done now.

PDavid · 2025-01-20T10:43:45Z

OK, now as suggested I reverted the change that requires the new commons-text dependency.

stoty · 2025-01-22T06:43:20Z

This looks mostly good to me, my only problem that this does not match the JIRA description.
At least half of the changes are not in JSPs.

PDavid · 2025-01-22T08:19:01Z

This looks mostly good to me, my only problem that this does not match the JIRA description. At least half of the changes are not in JSPs.

@stoty thanks, this is a very good point. I started with the deprecated calls in the JSP-s but then @NihalJain proposed to also replace the same org.apache.hadoop.util.StringUtils.humanReadableInt(long) call in all places.
So eventually now we have more changes not in the JSP-s. 🙈

Should I maybe revert those changes? Or rename the Jira ticket?
What do you all think?

stoty · 2025-01-22T08:39:27Z

Update the ticket and commit description.

- Replaced the usage of the following deprecated methods: - java.net.URLEncoder.encode(String) -> java.net.URLEncoder.encode(String, Charset) - StringUtils.humanReadableInt(long) -> StringUtils.TraditionalBinaryPrefix.long2String(long, "", 1): For this a new static util method is introduced: org.apache.hadoop.hbase.util.Strings.humanReadableInt - org.apache.hadoop.fs.FileSystem.getLength(Path) -> getFileStatus(Path).getLen() - org.apache.hadoop.hbase.ServerName.getStartcode() -> org.apache.hadoop.hbase.ServerName.getStartCode() - Also removed unused imports in the touched JSP files.

Apache-HBase · 2025-01-22T16:37:35Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 29s		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 11s		Maven dependency ordering for branch
+1 💚	mvninstall	2m 52s		master passed
+1 💚	compile	4m 43s		master passed
+1 💚	checkstyle	1m 5s		master passed
+1 💚	spotbugs	2m 50s		master passed
+1 💚	spotless	0m 41s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 11s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 49s		the patch passed
+1 💚	compile	4m 46s		the patch passed
+1 💚	javac	4m 46s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 8s		the patch passed
+1 💚	spotbugs	3m 27s		the patch passed
+1 💚	hadoopcheck	11m 0s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 43s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 33s		The patch does not generate ASF License warnings.
		45m 7s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6585/6/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6585
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux 50ae3af7d97e 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 / `cc52e79`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	85 (vs. ulimit of 30000)
modules	C: hbase-common hbase-server hbase-mapreduce hbase-diagnostics U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6585/6/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-01-22T20:21:40Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 35s		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 15s		Maven dependency ordering for branch
+1 💚	mvninstall	3m 33s		master passed
+1 💚	compile	2m 11s		master passed
+1 💚	javadoc	1m 21s		master passed
+1 💚	shadedjars	6m 5s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 13s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 53s		the patch passed
+1 💚	compile	1m 50s		the patch passed
+1 💚	javac	1m 50s		the patch passed
+1 💚	javadoc	1m 8s		the patch passed
+1 💚	shadedjars	5m 45s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	2m 15s		hbase-common in the patch passed.
-1 ❌	unit	210m 57s	/patch-unit-hbase-server.txt	hbase-server in the patch failed.
+1 💚	unit	19m 51s		hbase-mapreduce in the patch passed.
+1 💚	unit	4m 49s		hbase-diagnostics in the patch passed.
		269m 2s

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

PDavid commented Jan 8, 2025

View reviewed changes

hbase-server/src/main/resources/hbase-webapps/master/procedures.jsp Show resolved Hide resolved

PDavid commented Jan 8, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

PDavid marked this pull request as ready for review January 9, 2025 07:41