HADOOP-19329. Remove direct usage of sun.misc.Signal #7759
+181
−18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pan3793
changed the title
LuciferYang
reviewed
Jun 24, 2025
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
Outdated
Show resolved
Hide resolved
LuciferYang
reviewed
Jun 24, 2025
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
Show resolved
Hide resolved
LuciferYang
reviewed
Jun 24, 2025
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
Outdated
Show resolved
Hide resolved
LuciferYang
reviewed
Jun 24, 2025
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
Outdated
Show resolved
Hide resolved
LuciferYang
reviewed
Jun 24, 2025
| handler.handle(new Signal(args[0])); | ||
| return null; | ||
| } else { | ||
| return method.invoke(proxyObj, args); |
There was a problem hiding this comment.
Perhaps there's still an issue here... If someone invokes methods like toString(), equals(), hashCode(), etc., on the proxy object, and these invocations don't match the "handle" condition, they will enter the else branch and execute method.invoke(proxyObj, args). However, since proxyObj is the proxy object itself, this will again trigger the InvocationHandler, potentially leading to an infinite recursive call? Is my understanding correct?
There was a problem hiding this comment.
you are right, I should forward other method invocations to the proxied object, updated.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
cnauroth
reviewed
Jun 24, 2025
There was a problem hiding this comment.
@pan3793 , thank you for the patch. Sorry, I'm a bit confused about how this is working. I thought compilation failed because the classes are no longer present in the JDK. If they are accessible through reflection though, then I guess that means the classes are still there? If so, then why did compilation fail? (Not exported?)
I agree with the intuition to avoid additional dependencies if it's feasible.
| @@ -39,7 +38,7 @@ | |||
| @InterfaceAudience.Private | |||
| @InterfaceStability.Unstable | |||
| @SuppressWarnings("UseOfSunClasses") | |||
There was a problem hiding this comment.
Can this warning suppression be removed now?
|
@cnauroth If we don't pursue compiling Hadoop using a modern JDK with |
cnauroth
reviewed
Jun 25, 2025
There was a problem hiding this comment.
@pan3793 , thank you for the explanation! I'm in favor of the reflection-based approach here as opposed to a new dependency (which would cascade to the numerous projects dependent on Hadoop). I will be +1 pending resolution of a minor code review comment from me and resolution of Checkstyle issues.
I intend to leave this open a little longer though to see if there are different opinions.
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a dynamic wrapper to remove direct dependencies on sun.misc.Signal and updates existing components to use the new SignalUtil API.
- Adds
SignalUtilto handle signal creation, handling, and raising via reflection - Refactors
SignalLoggerandIrqHandlerto useSignalUtil.SignalandSignalUtil.Handlerinstead ofsun.misc.Signal/SignalHandler
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java | New utility class wrapping sun.misc.Signal dynamically |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalLogger.java | Updated to use SignalUtil.Signal and SignalUtil.Handler |
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/service/launcher/IrqHandler.java | Switched from direct Signal usage to SignalUtil |
Comments suppressed due to low confidence (2)
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java:66
- The inline comment incorrectly references
sun.misc.SignalHandler; thisdelegateholds asun.misc.Signalinstance. Please update the comment.
private final Object/* sun.misc.SignalHandler */ delegate;
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java:71
- Missing import for Preconditions; add
import org.apache.hadoop.util.Preconditions;at the top of the file.
Preconditions.checkNotNull(name);
| public String getName() { | ||
| return getNameMethod.invoke(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider adding an @Override annotation to the equals method for clarity and to catch potential signature mismatches.
Suggested change
| @Override |
Copilot uses AI. Check for mistakes.
| } | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider adding an @Override annotation to the hashCode method to align with best practices.
Suggested change
| @Override |
Copilot uses AI. Check for mistakes.
| public int hashCode() { | ||
| return delegate.hashCode(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider adding an @Override annotation to the toString method for consistency.
Suggested change
| @Override |
Copilot uses AI. Check for mistakes.
|
@adoroszlai Could you please help review this pr? Thank you very much! |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Alternative of #7145
How was this patch tested?
Pass existing UT:
with additional manual test to ensure
SignalLoggerworks properlyFor code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?