Uploaded image for project: 'Bigtop'
  1. Bigtop
  2. BIGTOP-2459

Add option to disable vmem check for NodeManager

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.1.0
    • Fix Version/s: 1.2.0
    • Component/s: deployment
    • Labels:
      None
    • Flags:
      Patch

      Description

      Virtual memory allocation can be aggressive (especially with Java 8). Expose the yarn.nodemanager.vmem-check-enabled config as a puppet config option to enable users to disable this check.

      1. BIGTOP-2459.patch
        2 kB
        Cory Johns
      2. BIGTOP-2459.patch
        2 kB
        Cory Johns
      3. BIGTOP-2459.patch
        2 kB
        Cory Johns
      4. BIGTOP-2459.patch
        2 kB
        Cory Johns

        Activity

        Hide
        cos Konstantin Boudnik added a comment -

        Don't you need to set the value of the variable in cluster.yaml view a hiera call? Then in the hadoop/init.pp you'll use that variable. Otherwise, I don't see how one can set the value to false and pass it into the class.

        Show
        cos Konstantin Boudnik added a comment - Don't you need to set the value of the variable in cluster.yaml view a hiera call? Then in the hadoop/init.pp you'll use that variable. Otherwise, I don't see how one can set the value to false and pass it into the class.
        Hide
        johnsca Cory Johns added a comment -

        We weren't sure what the correct pattern to parameterize this was. If the bigtop:: namespace is intended for params, would it be better to leave cluster.yaml out of this entirely and change the init.pp line to:

              $yarn_nodemanager_vmem_check_enabled = hiera('bigtop::yarn_nodemanager_vmem_check_enabled', false),
        

        Or should the value go through cluster.yaml to make it more easily discoverable?

        Show
        johnsca Cory Johns added a comment - We weren't sure what the correct pattern to parameterize this was. If the bigtop:: namespace is intended for params, would it be better to leave cluster.yaml out of this entirely and change the init.pp line to: $yarn_nodemanager_vmem_check_enabled = hiera('bigtop::yarn_nodemanager_vmem_check_enabled', false ), Or should the value go through cluster.yaml to make it more easily discoverable?
        Hide
        cos Konstantin Boudnik added a comment -

        cluster.yaml indeed includes all the defaults and lists the vars needed in the deployment. However, it should be left intact during the provisioning with all the changes to be expressed in site.yaml instead. What you suggest to do in the init.pp makes sense so the value changes could be done in site.yaml without touching cluster.pp and exposed in the class. Or to be left to its default set in the cluster.yaml if desired.

        Show
        cos Konstantin Boudnik added a comment - cluster.yaml indeed includes all the defaults and lists the vars needed in the deployment. However, it should be left intact during the provisioning with all the changes to be expressed in site.yaml instead. What you suggest to do in the init.pp makes sense so the value changes could be done in site.yaml without touching cluster.pp and exposed in the class. Or to be left to its default set in the cluster.yaml if desired.
        Hide
        johnsca Cory Johns added a comment -

        Updated the patch to use explicit hiera() call in init.pp

        Show
        johnsca Cory Johns added a comment - Updated the patch to use explicit hiera() call in init.pp
        Hide
        oflebbe Olaf Flebbe added a comment -

        Sorry, please do not use bigtop:: as the namespace of hadoop specific things.

        please use in cluster.yaml

        hadoop::common_yarn::yarn_nodemanager_vmem_check_enabled : true
        

        and add a parameter to the class common_yarn – like yarn_resourcemanager_ha_enabled, for instance. hiera will supply the cluster.yaml value to the class automatically.

        Show
        oflebbe Olaf Flebbe added a comment - Sorry, please do not use bigtop:: as the namespace of hadoop specific things. please use in cluster.yaml hadoop::common_yarn::yarn_nodemanager_vmem_check_enabled : true and add a parameter to the class common_yarn – like yarn_resourcemanager_ha_enabled , for instance. hiera will supply the cluster.yaml value to the class automatically.
        Hide
        cos Konstantin Boudnik added a comment -

        Ah! Thanks for a much better explanation, Olaf Flebbe!

        Show
        cos Konstantin Boudnik added a comment - Ah! Thanks for a much better explanation, Olaf Flebbe !
        Hide
        johnsca Cory Johns added a comment -

        New patch based on Olaf Flebbe's feedback. No bigtop:: namespace, implicit use of Hiera to populate the param, and default value given in cluster.yaml.

        Show
        johnsca Cory Johns added a comment - New patch based on Olaf Flebbe 's feedback. No bigtop:: namespace, implicit use of Hiera to populate the param, and default value given in cluster.yaml .
        Hide
        cos Konstantin Boudnik added a comment -

        Sorry Cory Johns, aren't you missing the hiera call in this line + $yarn_nodemanager_vmem_check_enabled, ??

        Show
        cos Konstantin Boudnik added a comment - Sorry Cory Johns , aren't you missing the hiera call in this line + $yarn_nodemanager_vmem_check_enabled, ??
        Hide
        johnsca Cory Johns added a comment -

        Per Olaf Flebbe's comment, my testing, and my reading of, e.g., the "Data Bindings" section of this blog post, Hiera should supply that parameter value implicitly from the value specified in cluster.yaml or overridden in site.yaml. However, as that blog post notes, there are potential "gotchas" with implicit parameter binding and maybe it's not the best practice? I'd like to defer to your and Olaf Flebbe's judgement here, though.

        Show
        johnsca Cory Johns added a comment - Per Olaf Flebbe 's comment, my testing, and my reading of, e.g., the "Data Bindings" section of this blog post , Hiera should supply that parameter value implicitly from the value specified in cluster.yaml or overridden in site.yaml . However, as that blog post notes, there are potential "gotchas" with implicit parameter binding and maybe it's not the best practice? I'd like to defer to your and Olaf Flebbe 's judgement here, though.
        Hide
        johnsca Cory Johns added a comment -

        Updated patch to move default, per review on BIGTOP-2458

        Show
        johnsca Cory Johns added a comment - Updated patch to move default, per review on BIGTOP-2458
        Hide
        johnsca Cory Johns added a comment -

        Updated patch to be in line with the recommendation from Olaf Flebbe's comment on BIGTOP-2458 (i.e., not render the stanza if the value is not explicitly set).

        Show
        johnsca Cory Johns added a comment - Updated patch to be in line with the recommendation from Olaf Flebbe 's comment on BIGTOP-2458 (i.e., not render the stanza if the value is not explicitly set).
        Hide
        kwmonroe Kevin W Monroe added a comment -

        +1 LGTM. Thanks for adjusting this patch in line with Olaf's recommendation in 2458.

        Show
        kwmonroe Kevin W Monroe added a comment - +1 LGTM. Thanks for adjusting this patch in line with Olaf's recommendation in 2458.
        Hide
        oflebbe Olaf Flebbe added a comment -

        Thanks!

        Show
        oflebbe Olaf Flebbe added a comment - Thanks!

          People

          • Assignee:
            johnsca Cory Johns
            Reporter:
            johnsca Cory Johns
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development