[jira] [Work logged] (GOBBLIN-1914) Add ability to determine App master class from runtime config

[ASF GitHub Bot (Jira)]

     [ 
https://issues.apache.org/jira/browse/GOBBLIN-1914?focusedWorklogId=881059&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-881059
 ]

ASF GitHub Bot logged work on GOBBLIN-1914:
-------------------------------------------

                Author: ASF GitHub Bot
            Created on: 21/Sep/23 01:32
            Start Date: 21/Sep/23 01:32
    Worklog Time Spent: 10m 
      Work Description: phet commented on code in PR #3781:
URL: https://github.com/apache/gobblin/pull/3781#discussion_r1332339649


##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java:
##########
@@ -342,7 +342,8 @@ public void initializeYarnClients(Config config) {
    * @throws IOException if there's something wrong launching the application
    * @throws YarnException if there's something wrong launching the application
    */
-  public void launch() throws IOException, YarnException, InterruptedException 
{
+  public void launch()

Review Comment:
   should it really be checked?  e.g. how likely is it that a caller would want 
to handle CNFE specifically?
   
   we might throw something more general, like IOE... or just wrap it in 
`RuntimeException`. looking at a few other uses of reflection for dynamic 
instance-construction, I don't see reflection-based failures showing up in the 
exception signature



##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnAppLauncher.java:
##########
@@ -801,23 +801,25 @@ private void addJobConfPackage(String jobConfPackagePath, 
Path destDir, Map<Stri
 
   @VisibleForTesting
   protected String buildApplicationMasterCommand(String applicationId, int 
memoryMbs) {
-    String appMasterClassName = GobblinApplicationMaster.class.getSimpleName();
+    String appMasterClass = ConfigUtils.getString(
+       config, GobblinYarnConfigurationKeys.APP_MASTER_CLASS, 
GobblinYarnConfigurationKeys.DEFAULT_APP_MASTER_CLASS);
+    String logFileName = GobblinApplicationMaster.class.getSimpleName();
     return new StringBuilder()
         
.append(ApplicationConstants.Environment.JAVA_HOME.$()).append("/bin/java")
         .append(" -Xmx").append((int) (memoryMbs * this.jvmMemoryXmxRatio) - 
this.jvmMemoryOverheadMbs).append("M")
         .append(" 
-D").append(GobblinYarnConfigurationKeys.JVM_USER_TIMEZONE_CONFIG).append("=").append(this.containerTimezone)
         .append(" 
-D").append(GobblinYarnConfigurationKeys.GOBBLIN_YARN_CONTAINER_LOG_DIR_NAME).append("=").append(ApplicationConstants.LOG_DIR_EXPANSION_VAR)
-        .append(" 
-D").append(GobblinYarnConfigurationKeys.GOBBLIN_YARN_CONTAINER_LOG_FILE_NAME).append("=").append(appMasterClassName).append(".").append(ApplicationConstants.STDOUT)
+        .append(" 
-D").append(GobblinYarnConfigurationKeys.GOBBLIN_YARN_CONTAINER_LOG_FILE_NAME).append("=").append(logFileName).append(".").append(ApplicationConstants.STDOUT)
         .append(" ").append(JvmUtils.formatJvmArguments(this.appMasterJvmArgs))
-        .append(" ").append(GobblinApplicationMaster.class.getName())
+        .append(" ").append(appMasterClass)
         .append(" 
--").append(GobblinClusterConfigurationKeys.APPLICATION_NAME_OPTION_NAME)
         .append(" ").append(this.applicationName)
         .append(" 
--").append(GobblinClusterConfigurationKeys.APPLICATION_ID_OPTION_NAME)
         .append(" ").append(applicationId)
         .append(" 
1>").append(ApplicationConstants.LOG_DIR_EXPANSION_VAR).append(File.separator).append(
-            appMasterClassName).append(".").append(ApplicationConstants.STDOUT)
+            logFileName).append(".").append(ApplicationConstants.STDOUT)

Review Comment:
   I'm up for whatever... but personally I can't think of situations where I 
would choose to override the very sensible default:
   ```
   Class.forName(appMasterClass).getSimpleName()
   ```



##########
gobblin-yarn/src/main/java/org/apache/gobblin/yarn/GobblinYarnConfigurationKeys.java:
##########
@@ -30,6 +30,9 @@ public class GobblinYarnConfigurationKeys {
   public static final String GOBBLIN_YARN_PREFIX = "gobblin.yarn.";
 
   // General Gobblin Yarn application configuration properties.
+  public static final String APP_MASTER_CLASS = GOBBLIN_YARN_PREFIX + 
"app.master.class";
+  public static final String DEFAULT_APP_MASTER_CLASS = 
GobblinApplicationMaster.class.getName();

Review Comment:
   these are in the same package, so potentially OK... but just noting this 
introduces a circular dependency





Issue Time Tracking
-------------------

    Worklog Id:     (was: 881059)
    Time Spent: 1h 20m  (was: 1h 10m)

> Add ability to determine App master class from runtime config
> -------------------------------------------------------------
>
>                 Key: GOBBLIN-1914
>                 URL: https://issues.apache.org/jira/browse/GOBBLIN-1914
>             Project: Apache Gobblin
>          Issue Type: Improvement
>            Reporter: Matthew Ho
>            Priority: Major
>          Time Spent: 1h 20m
>  Remaining Estimate: 0h
>




--
This message was sent by Atlassian Jira
(v8.20.10#820010)