- 
                Notifications
    You must be signed in to change notification settings 
- Fork 28.9k
[SPARK-29070][CORE] Make SparkLauncher log full spark-submit command line #25777
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
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.
I think you can just reflect this method and call sparkLauncher.createBuilder().command() as described in the mailing list. Not sure how much useful this API is in general yet.
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.
How about simply logging the command from SparkLauncher instead of adding a new public method, then?
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.
How is it differernt from setting SPARK_PRINT_LAUNCH_COMMAND to 1?
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.
That prints the command to stderr, which is not always easy to access (depending on the environment, etc.). Using the logging API gives clients more flexibility in controlling where the information appears (they can use the full power of log4j, for example).
73121ef    to
    047d74d      
    Compare
  
    | */ | ||
| public class SparkLauncher extends AbstractLauncher<SparkLauncher> { | ||
|  | ||
| // use JUL logger, relying on jul-to-slf4j bridge | 
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.
Unnecessary comment.
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.
Removed
        
          
                dev/.rat-excludes
              
                Outdated
          
        
      | vote.tmpl | ||
| SessionManager.java | ||
| SessionHandler.java | ||
| .python-version | 
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.
It seems odd to have this here. What is generating this file? It's definitely not being added by this PR. And other builds are not failing. So I think this deserves an explanation...
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.
It's a pyenv metadata file. I was using pyenv to manage Python versions locally for purposes of running the PySpark tests. It doesn't really need a license file since it's just a single line indicating the Python version. Nonetheless, it indeed has nothing to do with this change, so I'll remove it.
| ok to test | 
…line Log the full spark-submit command in SparkSubmit#launchApplication, using a JUL logger
047d74d    to
    0af4f88      
    Compare
  
    | Test build #111444 has finished for PR 25777 at commit  
 | 
| Test build #111442 has finished for PR 25777 at commit  
 | 
| retest this please | 
| Test build #111453 has finished for PR 25777 at commit  
 | 
| Merging to master. | 
Log the full spark-submit command in SparkSubmit#launchApplication
Adding .python-version (pyenv file) to RAT exclusion list
What changes were proposed in this pull request?
Original motivation here, expanded in the Jira.. In essence, we want to be able to log the full
spark-submitcommand being constructed bySparkLauncherWhy are the changes needed?
Currently, it is not possible to directly obtain this information from the
SparkLauncherinstance, which makes debugging and customer support more difficult.Does this PR introduce any user-facing change?
No
How was this patch tested?
coresbttests were executed. TheSparkLauncherSuite(where I added assertions to an existing test) was also checked. Within that,testSparkLauncherGetErroris failing, but that appears not to have been caused by this change (failing for me even on the parent commit of c18f849).