[GitHub] flink pull request #5172: [FLINK-8275] [Security] fix keytab local path in Y...

[tzulitai <gi...@git.apache.org>]

Github user tzulitai commented on a diff in the pull request:

    https://github.com/apache/flink/pull/5172#discussion_r165636080
  
    --- Diff: flink-yarn/src/main/java/org/apache/flink/yarn/YarnTaskManagerRunner.java ---
    @@ -142,19 +153,10 @@ public static void runYarnTaskManager(String[] args, final Class<? extends YarnT
     
     			SecurityUtils.install(sc);
     
    -			SecurityUtils.getInstalledContext().runSecured(new Callable<Object>() {
    -				@Override
    -				public Integer call() {
    -					try {
    -						TaskManager.selectNetworkInterfaceAndRunTaskManager(configuration, resourceId, taskManager);
    -					}
    -					catch (Throwable t) {
    -						LOG.error("Error while starting the TaskManager", t);
    -						System.exit(TaskManager.STARTUP_FAILURE_RETURN_CODE());
    -					}
    -					return null;
    -				}
    -			});
    +			if (mainRunner == null) {
    --- End diff --
    
    Not sure about this.
    This is basically adding a non-production code relevant path in for testing purposes (i.e., it is only ever non-null in the `YarnTaskManagerRunnerTest`).
    
    I think it would be better if we have a `protected createMainRunner(...)` method that can be overriden to inject the mock runner dependency for testing.


---