[GitHub] [incubator-dolphinscheduler] gabrywu edited a comment on pull request #2965: [Feature-2925][common] Add exitVal judge in OSUtils.exeCmd (#2925)

[GitBox <gi...@apache.org>]

gabrywu edited a comment on pull request #2965:
URL: https://github.com/apache/incubator-dolphinscheduler/pull/2965#issuecomment-645730853


   > > @yangyichao-mango What's purpose of this PR? Why are there so many irrelevant files to change ?
   > 
   > Thx a lot for your review @gabrywu .
   > 
   > This PR is essentially to get the abnormal exit value caused by the error (exceptions caused by permissions, etc.) when the worker starts to create the working directory, to judge if the user info and work dir is created successfully.
   > 
   > But after my change, I found that if the worker failed to create the working directory, the exception it threw would be caught, only recorded in the log of the worker node, not in the log of a single task instance. Although the task will fail at the end of execution, the exception log is very few and it is difficult to locate the exception (permission exception, etc.) directly.
   > 
   > In my opinion, I understand that this error should fail fast, or at least recorded by logs. So in order to match the change of my exitval judgment work, I put the initialization change of tasklogger to the beginning of `TaskExecuteProcessor`. And I understand that the task logger should record all the logs related to the task, not only the task runtime logs.
   > 
   > If I have any misunderstanding, please give me suggestions.
   > 
   > 非常感谢你的review。
   > 
   > 这个pr本质上是为了获取在worker执行task时创建工作目录时的错误（由于权限导致的问题等）值，来判断工作目录，用户权限是否创建成功。
   > 
   > 但是在改动完成后，我发现如果worker创作工作目录失败其抛出的异常会被catch住，异常信息只会在worker节点的日志中被记录下来，不会在单个task的日志中记录下来，这相当于创建工作目录相关的所有错误信息完全不会被直接外显给用户。虽然最后task执行的时候会失败，但是异常日志非常少，很难直接定位到工作目录中的异常（权限异常等）。
   > 
   > 我理解这种错误应该fail-fast，或者至少有日志来记录，所以为了配合我这次针对exitval判断的改动，我就把taskLogger的初始化改动到了最前面，用来记录涉及到这个task的所有日志。并且我理解taskLogger的原本目的就是用来记录所有涉及到这个task的日志，不仅仅是task运行时的日志。
   > 
   > 如果我有理解不正确的地方，敬请指出。
   
   You'd better give a suitable title to this issue, otherwise others may have a misunderstanding. If you want to fix different issue,  different PRs should be created


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org