Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 1.0.0
    • Component/s: deployment
    • Labels:

      Description

      Duplicates BIGTOP-1799

      and introduces a whole range of new variables to customize hue..

      Timezones, Authentication (Specifically LDAP), HA Resource Managers ..,

      1. BIGTOP-1872.1.patch
        21 kB
        Olaf Flebbe
      2. BIGTOP-1872.2.patch
        21 kB
        Olaf Flebbe
      3. BIGTOP-1872.3.patch
        21 kB
        Olaf Flebbe

        Activity

        Hide
        oflebbe Olaf Flebbe added a comment -

        Can we get in this as well ?

        Show
        oflebbe Olaf Flebbe added a comment - Can we get in this as well ?
        Hide
        cos Konstantin Boudnik added a comment -

        If it ready, can be committed, and isn't overly destructive - sure Sorry I can't do much of the reviewing - I have no clue about HUE

        Show
        cos Konstantin Boudnik added a comment - If it ready, can be committed, and isn't overly destructive - sure Sorry I can't do much of the reviewing - I have no clue about HUE
        Hide
        petersla Peter Slawski added a comment -

        I took a look, here are my comments:

        bigtop-deploy/puppet/modules/hue/manifests/init.pp:

        • The parameters for the Hue::Server class could be indented consistently. Since there are a lot of them now, maybe one parameter per line would be easier to read (like in the Hadoop::Common_hdfs class).
        • The Hue service shouldn't start until all hue packages are installed. Otherwise, depending on puppet's ordering hue server could start before some Hue apps are installed (when $hue_apps is not set to "all" or "none"). Then, those apps wouldn't be available until hue-server is restarted.
        • Should the security app be blacklisted by default along with Impala?

        bigtop-deploy/puppet/modules/hue/templates/hue.ini:

        • It would be nice to have the secret_key be randomly generated by the template.
        • The first occurrence of sync_groups_on_login can be removed as that parameter as been moved in HUE-2671.
        • Perhaps the nested ifs under <% if @ldap_url %> should be indented to make it clear that they are nested.
        • The commented block for ldap_username_pattern is repeated twice. The newly added block is in the else block of <% if @user_filter %>
        • A committed out line for security_enabled has been added, but below it, it is already defined.
        • Near the end I see -%> being used to suppress leading line breaks. It would be nice if it was also used in earlier ifs to keep formatting new lines between parameters and comments consistent.
        Show
        petersla Peter Slawski added a comment - I took a look, here are my comments: bigtop-deploy/puppet/modules/hue/manifests/init.pp : The parameters for the Hue::Server class could be indented consistently. Since there are a lot of them now, maybe one parameter per line would be easier to read (like in the Hadoop::Common_hdfs class). The Hue service shouldn't start until all hue packages are installed. Otherwise, depending on puppet's ordering hue server could start before some Hue apps are installed (when $hue_apps is not set to "all" or "none"). Then, those apps wouldn't be available until hue-server is restarted. Should the security app be blacklisted by default along with Impala? bigtop-deploy/puppet/modules/hue/templates/hue.ini : It would be nice to have the secret_key be randomly generated by the template. The first occurrence of sync_groups_on_login can be removed as that parameter as been moved in HUE-2671 . Perhaps the nested ifs under <% if @ldap_url %> should be indented to make it clear that they are nested. The commented block for ldap_username_pattern is repeated twice. The newly added block is in the else block of <% if @user_filter %> A committed out line for security_enabled has been added, but below it, it is already defined. Near the end I see -%> being used to suppress leading line breaks. It would be nice if it was also used in earlier ifs to keep formatting new lines between parameters and comments consistent.
        Hide
        oflebbe Olaf Flebbe added a comment -

        Thanks Peter Slawski for your detailed review!

        I adressed all but one of your comments in the updated patch.

        The secret_key to be generated needs AFAIK more ground work in puppet, so I left it out for now.

        Yep, the security app does not make sense because apache sentry is missing from bigtop.

        Show
        oflebbe Olaf Flebbe added a comment - Thanks Peter Slawski for your detailed review! I adressed all but one of your comments in the updated patch. The secret_key to be generated needs AFAIK more ground work in puppet, so I left it out for now. Yep, the security app does not make sense because apache sentry is missing from bigtop.
        Hide
        petersla Peter Slawski added a comment -

        Thanks for addressing my comments.

        AFAIK the following line shouldn't be necessary as in cluster.pp#L171, the Hadoop::Httpfs class would dependson the Hue::Server class. Both classes contain their service resource, so the hadoop-httpfs service would be started before the hue service.

             Kerberos::Host_keytab <| title == "hue" |> -> Service["hue"]
        -
        +    Service<| title == "hadoop-httpfs" |> -> Service["hue"]
           }
        

        Also, I didn't notice this before, but the license for the hue.ini template is being removed. Is this intentional?

        Show
        petersla Peter Slawski added a comment - Thanks for addressing my comments. AFAIK the following line shouldn't be necessary as in cluster.pp#L171 , the Hadoop::Httpfs class would dependson the Hue::Server class. Both classes contain their service resource, so the hadoop-httpfs service would be started before the hue service. Kerberos::Host_keytab <| title == "hue" |> -> Service[ "hue" ] - + Service<| title == "hadoop-httpfs" |> -> Service[ "hue" ] } Also, I didn't notice this before, but the license for the hue.ini template is being removed. Is this intentional?
        Hide
        oflebbe Olaf Flebbe added a comment -

        The dependency on hadoop-httpfs service is intentional. I did see hue problems in the past when hadoop-httpfs is not started before hue.

        The missing header is intentional, since I used the hue.ini from the template within the hue distribution as a boilerplate template. The license header is removed within this distribution file with 3.8.x. Not sure whether this is intentional of upstream authors.

        Show
        oflebbe Olaf Flebbe added a comment - The dependency on hadoop-httpfs service is intentional. I did see hue problems in the past when hadoop-httpfs is not started before hue. The missing header is intentional, since I used the hue.ini from the template within the hue distribution as a boilerplate template. The license header is removed within this distribution file with 3.8.x. Not sure whether this is intentional of upstream authors.
        Hide
        petersla Peter Slawski added a comment -

        Shouldn't this dependency be made in cluster.pp as httpfs may not be included in the components list?

        Show
        petersla Peter Slawski added a comment - Shouldn't this dependency be made in cluster.pp as httpfs may not be included in the components list?
        Hide
        oflebbe Olaf Flebbe added a comment -

        Ok, now I got it. You are right. Have to update.

        Show
        oflebbe Olaf Flebbe added a comment - Ok, now I got it. You are right. Have to update.
        Hide
        oflebbe Olaf Flebbe added a comment -

        Yep you are right. Removing this line should be sufficient.

        Show
        oflebbe Olaf Flebbe added a comment - Yep you are right. Removing this line should be sufficient.
        Hide
        petersla Peter Slawski added a comment -

        +1 Looks good!

        Show
        petersla Peter Slawski added a comment - +1 Looks good!
        Hide
        oflebbe Olaf Flebbe added a comment - - edited

        Thank you very much for reviewing! Committed right now.

        Show
        oflebbe Olaf Flebbe added a comment - - edited Thank you very much for reviewing! Committed right now.

          People

          • Assignee:
            oflebbe Olaf Flebbe
            Reporter:
            oflebbe Olaf Flebbe
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development