[ 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: {code} 264 len = strlen(line); 265 buffer = (char *) calloc(len + 1, sizeof(char)); 266 strncpy(buffer, line, len); {code} 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. {code} 269 //if no equals is found ignore this line, can be an empty line also {code} 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 {code} 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)); {code} 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. {code} 316 if (section->kv_pairs[section->size]) { {code} I think this check is redundant {code} 366 static void populate_section_fields(FILE *conf_file, struct section *section) { {code} Just a note: you would probably save some memory and extra copies, if this was implemented as a state machine. {code} 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 } {code} 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. {code} 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 } {code} Just to be safe, I would set the freed pointers to NULL here. {code} 478 fseek(conf_file, 0, SEEK_SET); {code} You might want to add a comment // populate any entries in the newer format (sections) {code} 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) { {code} 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 (v6.4.14#64029) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org