hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Miklos Szegedi (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-6033) Add support for sections in container-executor configuration file
Date Mon, 31 Jul 2017 19:43:00 GMT

    [ https://issues.apache.org/jira/browse/YARN-6033?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16107847#comment-16107847

Miklos Szegedi commented on YARN-6033:

Thank you, [~vvasudev] for the patch. I have some questions/comments
is_section_start_line and get_section_name should handle leading and trailing spaces, just
like the current behavior of the configuration lines.
Other comments:
264      len = strlen(line);
265      buffer = (char *) calloc(len + 1, sizeof(char));
266      strncpy(buffer, line, len);
You copy the value into the buffer right after allocation. If you do calloc to put the trailing
\0 there, it might be faster to do that explicitly, because this code fills the buffer twice.
269      //if no equals is found ignore this line, can be an empty line also
This is a little bit controversial. I would ignore a line trimmed to empty and return a configuration
error for anything else. It helps with debugging config issues
306      if ((section->size + 1) % MAX_SIZE == 0) {
307        section->kv_pairs = (struct kv_pair **) realloc(
308            section->kv_pairs,
309            sizeof(struct kv_pair *) * (MAX_SIZE + section->size));
Is not it a better practice to do the increase of the kv buffer before we add a new item?
It might happen that we have MAX_SIZE items, then we allocate MAX_SIZE*2. That might also
simplify the code since we do not need to allocate in the allocate_section.
316      if (section->kv_pairs[section->size]) {
I think this check is redundant
    366    static void populate_section_fields(FILE *conf_file, struct section *section) {
Just a note: you would probably save some memory and extra copies, if this was implemented
as a state machine.
376              if (section->name != NULL) {
377                if (read_section_entry(line, section) < 0) {
378                  fprintf(ERRORFILE, "Error parsing line %s", line);
379                  exit(INVALID_CONFIG_FILE);
380                }
381              }
This code needs an else exiting with an error (kv pair without section), or am I missing something?
In that case it might need to be commented.
294      if (equaltok == NULL) {
295        free((void *) section->kv_pairs[section->size]->key);
296        free((void *) section->kv_pairs[section->size]);
297        return 2;
298      }
Just to be safe, I would set the freed pointers to NULL here.
478      fseek(conf_file, 0, SEEK_SET);
You might want to add a comment // populate any entries in the newer format (sections)
421     * @param free_section free the second section if set
422     */
423    static void merge_sections(struct section *section1, struct section *section2, const
int free_section) {
If free_section is set, who will free section->name? BTW free_section is always 1, do we
really need the parameter?

> Add support for sections in container-executor configuration file
> -----------------------------------------------------------------
>                 Key: YARN-6033
>                 URL: https://issues.apache.org/jira/browse/YARN-6033
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: nodemanager
>            Reporter: Varun Vasudev
>            Assignee: Varun Vasudev
>         Attachments: YARN-6033.003.patch, YARN-6033.004.patch, YARN-6033.005.patch, YARN-6033.006.patch,
YARN-6033-YARN-5673.001.patch, YARN-6033-YARN-5673.002.patch

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org

View raw message