hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Varun Vasudev (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (YARN-6033) Add support for sections in container-executor configuration file
Date Wed, 02 Aug 2017 12:52:00 GMT

     [ 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-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message