Skip to content

HBASE-28898. Use reflection to access recoverLease(), setSafeMode() APIs. - #6342

Merged
jojochuang merged 10 commits into
apache:masterfrom
jojochuang:ozone_support_reflection
Oct 23, 2024
Merged

HBASE-28898. Use reflection to access recoverLease(), setSafeMode() APIs.#6342
jojochuang merged 10 commits into
apache:masterfrom
jojochuang:ozone_support_reflection

Conversation

@jojochuang

@jojochuangjojochuang commented Oct 4, 2024

Copy link
Copy Markdown
Contributor
  • use reflection to access these Hadoop APIs.
  • added more UT coverage. Tested a few UTs with Hadoop 3.3.5.

TODO:

  • Test with Hadoop 3.3.6, 3.4.0.
  • Test with Ozone 2.0.0-SNAPSHOT.

@jojochuang
Use reflection to access recoverlease(), setSafeMode() APIs.
49f4fa7
Change-Id: I1a7b7bad0e8967553490b70439b18f8ddfed6d14
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@jojochuang
Fix checkstyle
1585209
Change-Id: Ibf0ded6e104e9ed16f887f8411f4816eab8cea00
@jojochuang
spotless
8bc2b03
Change-Id: I351226ea55545dea309c7ed33c78c338cb42c07e
@jojochuang

jojochuang commented Oct 4, 2024

Copy link
Copy Markdown
ContributorAuthor

javac warning is unrelated.
[WARNING] /home/jenkins/jenkins-home/workspace/Base-PreCommit-GitHub-PR_PR-6342/yetus-general-check/src/hbase-asyncfs/src/test/java/org/apache/hadoop/hbase/util/TestRecoverLeaseFSUtils.java:[74,13] [NarrowCalculation] This product of integers could overflow before being implicitly cast to a long.

@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@jojochuangjojochuang mentioned this pull request Oct 4, 2024
Open
stoty
stoty reviewed Oct 9, 2024
View reviewed changes
Comment threadhbase-asyncfs/src/main/java/org/apache/hadoop/hbase/util/RecoverLeaseFSUtils.java Outdated
Comment threadhbase-asyncfs/src/main/java/org/apache/hadoop/hbase/util/RecoverLeaseFSUtils.java Outdated
Comment threadhbase-asyncfs/src/main/java/org/apache/hadoop/hbase/util/RecoverLeaseFSUtils.java Outdated
Comment threadhbase-asyncfs/src/main/java/org/apache/hadoop/hbase/util/RecoverLeaseFSUtils.java Outdated
@jojochuang
Update based on review comments.
441e54a
(1) Use FileSystem class instead of Object for file system object.
(2) Added one more test case to properly test reflection code.
Change-Id: I0710ae1023d81f68fb18ec3f8dd5b228d90df21e
@jojochuang
Fix java doc.
8b48401
Change-Id: I5e29bacec957d76555a0ff7483a462b614a8f3f5
@Apache-HBase

This comment has been minimized.

Sign in to view
@jojochuang
Spotless
ad06438
Change-Id: Ie0b74f4801de33e0683e97e234cad9fcf13e985a
@jojochuang
Fix compilation error.
e983c82
Change-Id: I4dd951945920220678677acefd27710eaf0f5658
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@stoty

stoty commented Oct 16, 2024

Copy link
Copy Markdown
Contributor

Looks good to me.
Please fix the spotless and other warnings (unless unrelated).

I am a bit worried about the runtime errors.
I have not dug through the API, but I assume those errors can never happen with a released Hadoop version, right ?

@jojochuang

jojochuang commented Oct 16, 2024

Copy link
Copy Markdown
ContributorAuthor

The unit test failures do not reproduce in my local tree, so assuming flaky. But I wonder if the this change makes them easier to fail. Will dig into it.

@jojochuang
Fix spotless and javac warnings.
8f7d058
Change-Id: Iec1149e25f6a8ac3f5fe4d2eb7287af9c0c75021
@jojochuang

jojochuang commented Oct 16, 2024

Copy link
Copy Markdown
ContributorAuthor

The spotbugs warning is unrelated. Will open another jira to track it.

stoty
stoty reviewed Oct 16, 2024
View reviewed changes

@stotystoty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot to send my pending comments.
Adding them now.

Comment threadhbase-asyncfs/src/main/java/org/apache/hadoop/hbase/util/RecoverLeaseFSUtils.java Outdated
private static Method recoverLeaseMethod = null;
public static final String LEASE_RECOVERABLE_CLASS_NAME = "org.apache.hadoop.fs.LeaseRecoverable";
static {
LOG.debug("RecoverLeaseFSUtils loaded");

@stotystotyOct 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This should be after initializeRecoverLeaseMethod

@stotystotyOct 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this log message before the actual initialization is misleading.
Is there a specific reason for this ordering ?

Alternatively change the message to "initializing RecoverLeaseFSUtils"

Comment threadhbase-asyncfs/src/main/java/org/apache/hadoop/hbase/util/RecoverLeaseFSUtils.java
LOG.debug(
"LeaseRecoverable interface not in the classpath, this means Hadoop 3.3.5 or below.");
try {
recoverLeaseMethod = DistributedFileSystem.class.getMethod("recoverLease", Path.class);

@stotystotyOct 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect to have this on all supported releases, right ?

@stotystotyOct 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe log an error message to explain that this should never happen ?

Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
.invoke(dfs, safeModeGet, true);
return (Boolean) ret;
} catch (InvocationTargetException | IllegalAccessException | NoSuchMethodException e) {
throw new RuntimeException(e);

@stotystotyOct 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no Hadoop release where we expect to get here, right ?

@stotystotyOct 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe log an error message to explain that this should never happen ?

Comment threadhbase-server/src/main/java/org/apache/hadoop/hbase/util/FSUtils.java
if (isDistributedFileSystem(dfs)) {
return ((DistributedFileSystem) dfs).setSafeMode(SAFEMODE_GET, true);
} else {
try {

@stotystotyOct 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just return false here if we could not find the method previously ?
i.e. if this is an older Hadoop, and this is called with say, LocalFilesystem ?

@jojochuangjojochuangOct 16, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before calling isInSafeMode() it calls supportSafeMode(). If it's LocalFileSystem it will not enter isInSafeMode().

@jojochuang
Improve error message if something unexpected happens using reflection.
d75d68f
Change-Id: Iee3f259a46b5f12ed5948c61738a19ef77e29b5a
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@Apache-HBase

This comment has been minimized.

Sign in to view
@jojochuang

jojochuang commented Oct 16, 2024

Copy link
Copy Markdown
ContributorAuthor

https://issues.apache.org/jira/browse/HBASE-28918 for the spotbug error

@Apache-HBase

This comment has been minimized.

Sign in to view
stoty
stoty approved these changes Oct 17, 2024
View reviewed changes

@stotystoty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM

I have kicked off a new CI run

stoty
stoty suggested changes Oct 17, 2024
View reviewed changes

@stotystoty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The log message in the static initializer is still misleading.

@Apache-HBase

This comment has been minimized.

Sign in to view
@stoty

stoty commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

Please also re-run spotless:apply, the new log message does not comply with the spotless rules.

@Apache-HBase

This comment has been minimized.

Sign in to view
@jojochuang
Fix spotless warning and fix log messages.
07c0831
Change-Id: Iee20335148f41592ed5b36c7c526a544fce6ed87
@jojochuang

jojochuang commented Oct 19, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review! Updated.

@Apache-HBase

Apache-HBase commented Oct 19, 2024

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 15sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+0 🆗codespell0m 0scodespell was not available.
+0 🆗detsecrets0m 0sdetect-secrets was not available.
+1 💚@author0m 0sThe patch does not contain any @author tags.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
_ master Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for branch
+1 💚mvninstall3m 0smaster passed
+1 💚compile3m 22smaster passed
+1 💚checkstyle0m 47smaster passed
-1 ❌spotbugs1m 34s/branch-spotbugs-hbase-server-warnings.htmlhbase-server in master has 1 extant spotbugs warnings.
+1 💚spotless0m 44sbranch has no errors when running spotless:check.
_ Patch Compile Tests _
+0 🆗mvndep0m 11sMaven dependency ordering for patch
+1 💚mvninstall2m 56sthe patch passed
+1 💚compile3m 23sthe patch passed
+1 💚javac3m 23sthe patch passed
+1 💚blanks0m 0sThe patch has no blanks issues.
+1 💚checkstyle0m 9sThe patch passed checkstyle in hbase-asyncfs
+1 💚checkstyle0m 37shbase-server: The patch generated 0 new + 10 unchanged - 1 fixed = 10 total (was 11)
+1 💚spotbugs2m 15sthe patch passed
+1 💚hadoopcheck10m 25sPatch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚spotless0m 44spatch has no errors when running spotless:check.
_ Other Tests _
+1 💚asflicense0m 20sThe patch does not generate ASF License warnings.
38m 11s
SubsystemReport/Notes
DockerClientAPI=1.47 ServerAPI=1.47 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6342/8/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#6342
Optional Testsdupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
unameLinux 3b1f0123e8d9 5.4.0-192-generic #212-Ubuntu SMP Fri Jul 5 09:47:39 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 07c0831
Default JavaEclipse Adoptium-17.0.11+9
Max. process+thread count84 (vs. ulimit of 30000)
modulesC: hbase-asyncfs hbase-server U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6342/8/console
versionsgit=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase

Apache-HBase commented Oct 19, 2024

Copy link
Copy Markdown

🎊 +1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 30sDocker mode activated.
-0 ⚠️yetus0m 2sUnprocessed 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 🆗mvndep0m 21sMaven dependency ordering for branch
+1 💚mvninstall2m 59smaster passed
+1 💚compile1m 11smaster passed
+1 💚javadoc0m 38smaster passed
+1 💚shadedjars5m 34sbranch has no errors when building our shaded downstream artifacts.
_ Patch Compile Tests _
+0 🆗mvndep0m 12sMaven dependency ordering for patch
+1 💚mvninstall3m 0sthe patch passed
+1 💚compile1m 12sthe patch passed
+1 💚javac1m 12sthe patch passed
+1 💚javadoc0m 38sthe patch passed
+1 💚shadedjars5m 34spatch has no errors when building our shaded downstream artifacts.
_ Other Tests _
+1 💚unit1m 0shbase-asyncfs in the patch passed.
+1 💚unit210m 28shbase-server in the patch passed.
238m 4s
SubsystemReport/Notes
DockerClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6342/8/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR#6342
Optional Testsjavac javadoc unit compile shadedjars
unameLinux 16057e79f534 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 toolmaven
Personalitydev-support/hbase-personality.sh
git revisionmaster / 07c0831
Default JavaEclipse Adoptium-17.0.11+9
Test Resultshttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6342/8/testReport/
Max. process+thread count4902 (vs. ulimit of 30000)
modulesC: hbase-asyncfs hbase-server U: .
Console outputhttps://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6342/8/console
versionsgit=2.34.1 maven=3.9.8
Powered byApache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

stoty
stoty approved these changes Oct 19, 2024
View reviewed changes

@stotystoty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 LGTM

@jojochuang
jojochuang merged commit 11c0742 into apache:masterOct 23, 2024
@jojochuang

jojochuang commented Oct 23, 2024

Copy link
Copy Markdown
ContributorAuthor

Merged. Thanks @stoty

jojochuang added a commit to jojochuang/hbase that referenced this pull request Oct 23, 2024
@jojochuang
HBASE-28898. Use reflection to access recoverLease(), setSafeMode() A…
f373405
…PIs. (apache#6342)
(cherry picked from commit 11c0742)
jojochuang added a commit to jojochuang/hbase that referenced this pull request Oct 23, 2024
@jojochuang
HBASE-28898. Use reflection to access recoverLease(), setSafeMode() A…
d2f0547
…PIs. (apache#6342)
(cherry picked from commit 11c0742)
(cherry picked from commit f373405)
jojochuang added a commit to jojochuang/hbase that referenced this pull request Oct 23, 2024
@jojochuang
HBASE-28898. Use reflection to access recoverLease(), setSafeMode() A…
5db43f7
…PIs. (apache#6342)
(cherry picked from commit 11c0742)
(cherry picked from commit f373405)
(cherry picked from commit d2f0547)
jojochuang added a commit that referenced this pull request Oct 25, 2024
@jojochuang
HBASE-28898. Use reflection to access recoverLease(), setSafeMode() A…
1767de5
…PIs. (#6342) (#6392)
(cherry picked from commit 11c0742)
(cherry picked from commit f373405)
(cherry picked from commit d2f0547)
jojochuang added a commit that referenced this pull request Oct 25, 2024
@jojochuang
HBASE-28898. Use reflection to access recoverLease(), setSafeMode() A…
0aa1c8d
…PIs. (#6342) (#6391)
(cherry picked from commit 11c0742)
jojochuang added a commit that referenced this pull request Oct 25, 2024
@jojochuang
HBASE-28898. Use reflection to access recoverLease(), setSafeMode() A…
2724054
…PIs. (#6342) (#6390)
(cherry picked from commit 11c0742)
(cherry picked from commit f373405)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@stotystotystoty approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jojochuang@Apache-HBase@stoty