[
https://issues.apache.org/jira/browse/YARN-6033?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Varun Vasudev updated YARN-6033:
--------------------------------
Attachment: YARN-6033.007.patch
bq. is_section_start_line and get_section_name should handle leading and
trailing spaces, just like the current behavior of the configuration lines.
I would prefer not to do this, we shouldn't allow section starts to be placed
anywhere on the line, nor should we strip trailing spaces.
{noformat}
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.
{noformat}
Fair point, but I prefer to use calloc. The speed shouldn't be a big deal for
us.
{noformat}
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
{noformat}
Agreed, fixed.
{noformat}
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.
{noformat}
Good point, fixed.
{noformat}
316 if (section->kv_pairs[section->size]) {
I think this check is redundant
{noformat}
Yep, fixed.
{noformat}
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.
{noformat}
Yeah, I figured for now I'd keep the logic simple.
{noformat}
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.
{noformat}
Yep, I missed that - fixed.
{noformat}
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.
{noformat}
Fixed.
{noformat}
478 fseek(conf_file, 0, SEEK_SET);
You might want to add a comment // populate any entries in the newer format
(sections)
{noformat}
Added the comment but removed the fseek because we don't actually need it.
{noformat}
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?
{noformat}
Nice catch on the name being freed, fixed. I think at some point for some
testing scenarios we might need it. If you feel strongly about it, I can remove
it.
> 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.007.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
