[
https://issues.apache.org/jira/browse/HADOOP-4930?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12658869#action_12658869
]
Hemanth Yamijala commented on HADOOP-4930:
------------------------------------------
Some comments:
- Include ASF header in all files.
- Returning different error codes might help, rather than return -1 for all
types of errors.
- Can we change the LOGFILE through an option. By default, it points to stderr
- There is a check in get_user_details that seems redundant. Since this is a
short lived executable and the user details are retrieved right at the
beginning and not modified later, the check there doesn't seem necessary.
- In main.c, commandArguments is unnecessary. Since they are read only strings,
you could directly use the argv pointers.
- mallocs are not allocating an additional byte for null-termination.
- At a couple of places, return values for APIs and system calls are not being
checked - for e.g. malloc, fopen, strtok, etc.
- In get_configs, the temp buffer doesn't need to be malloced. It can be a
fixed size buffer. In general, please review and try to reduce the number of
dynamic allocations where possible.
- Please ensure that the conf file parsing works even in face of invalid inputs
in the conf file. Invalid lines should be either discarded or the exe should
fail on such inputs. If we choose to exit on failures, supporting a # to
indicate commented lines would be good.
- Define constants for numeric literals
- For the small number of config items we have, I don't think maintaining them
sorted is necessary. Likewise, search can be a simple linear scan.
- We seem to be holding only 10 values in the configuration. This is lot for
now, but shouldn't this be dynamic ?
- We don't need to malloc again for the configuration keys and values. Since
the values are read only, pointers to them can be returned where needed. And
this structure is not being freed at the end of execution.
- In the function to check tt root, I think we should check for an exact match
of the strings, strncmp might make substrings pass
- For the first version, where file and directory permissions aren't being
guarded, we'll need to set a umask to create files readable by all.
- hadoop.temp.dir should be a list of paths.
- In get_task_file_path, additional memory is being allocated for the task id.
- Some debug prints are going to stderr, and not the LOGFILE.
- In the case of launching JVMs, any cleanup should be done before the call to
exec the script, because exec would not return if successful.
- Please include some more documentation about the functions, and the files in
general.
- Error messages can give some more information to users about the error.
> Implement setuid executable for Linux to assist in launching tasks as job
> owners
> --------------------------------------------------------------------------------
>
> Key: HADOOP-4930
> URL: https://issues.apache.org/jira/browse/HADOOP-4930
> Project: Hadoop Core
> Issue Type: Sub-task
> Components: mapred
> Reporter: Hemanth Yamijala
> Assignee: Sreekanth Ramakrishnan
> Attachments: HADOOP-4930-1.patch, HADOOP-4930-2.patch,
> HADOOP-4930-3.patch
>
>
> HADOOP-4490 tracks the design and implementation of launching tasks as job
> owners. As per discussion there, the proposal is to implement a setuid
> executable for Linux that would be launched by the tasktracker to run tasks
> as the job owners. In this task we will track the implementation of the
> setuid executable. HADOOP-4490 will be used to track changes in the
> tasktracker.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
