- 
                Notifications
    You must be signed in to change notification settings 
- Fork 9.1k
HADOOP-19329. Remove direct usage of sun.misc.Signal #7759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
        
          
                hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
          
            Show resolved
            Hide resolved
        
              
          
                hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | handler.handle(new Signal(args[0])); | ||
| return null; | ||
| } else { | ||
| return method.invoke(proxyObj, args); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
        
          
                ...ommon-project/hadoop-common/src/main/java/org/apache/hadoop/service/launcher/IrqHandler.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | @cnauroth  If we don't pursue compiling Hadoop using a modern JDK with   | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 now, though as discussed, I will leave it open a little longer in case other want to comment. Thank you @pan3793 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Signaldynamically | 
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalLogger.java | Updated to use SignalUtil.SignalandSignalUtil.Handler | 
| hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/service/launcher/IrqHandler.java | Switched from direct Signalusage toSignalUtil | 
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);
        
          
                hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
          
            Show resolved
            Hide resolved
        
              
          
                hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
          
            Show resolved
            Hide resolved
        
              
          
                hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SignalUtil.java
          
            Show resolved
            Hide resolved
        
      | @adoroszlai Could you please help review this pr? Thank you very much! | 
| 💔 -1 overall 
 
 This message was automatically generated. | 
| @pan3793 Thanks for the contribution! Merged into trunk. We have already waited for a long time, let's merge this PR into the trunk branch. | 
Description of PR
JIRA: HADOOP-19329. Remove direct usage of sun.misc.Signal.
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?