----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64594/#review193825 -----------------------------------------------------------
Fix it, then Ship it! ambari-common/src/main/python/resource_management/libraries/functions/cluster_settings.py Lines 45 (patched) <https://reviews.apache.org/r/64594/#comment272459> Hi, Eventually it might never happen that cluster_settings is None, but if it does then this would be a Logger.error and return None instead of Logger.info because it will throw an error at Line 54 if we continue with the function. Looks like we have this in Line 78 so need to have the same behaviour here. Thanks ambari-common/src/main/python/resource_management/libraries/functions/cluster_settings.py Lines 53 (patched) <https://reviews.apache.org/r/64594/#comment272460> What is the format that the calling function is expected to pass "setting_names" as? Do we need to do set(setting_names) if it is already being passed as a set? ambari-common/src/main/python/resource_management/libraries/functions/stack_settings.py Lines 50 (patched) <https://reviews.apache.org/r/64594/#comment272461> return None ambari-common/src/main/python/resource_management/libraries/functions/stack_settings.py Lines 58 (patched) <https://reviews.apache.org/r/64594/#comment272462> Remove set() - Madhuvanthi Radhakrishnan On Dec. 14, 2017, 9:02 a.m., Swapan Shridhar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64594/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2017, 9:02 a.m.) > > > Review request for Ambari, Jayush Luniya and Madhuvanthi Radhakrishnan. > > > Bugs: AMBARI-22649 > https://issues.apache.org/jira/browse/AMBARI-22649 > > > Repository: ambari > > > Description > ------- > > Background : AMBARI-22198 added "stack settings", and AMBARI-22196 introduced > "cluster settings" in Ambari. > > **==========================================================================================** > **Library for querying _clusterSettings_ and _stackSettings_ for its contents > in command*.json.** > **==========================================================================================** > > One should be able to query for a given **clusterSettings** or > **stackSettings**: > - by passing in the setting name(one or more) in order to get it back as > key-value map, or > - just get the value back for a passed-in setting. > > > **Functions for clusterSettings:** > **------------------------------** > - **get_cluster_setting_entries(setting_names)** : > -- Retrieves the passed-in cluster setting entr(y/ies) and their values > as a map. > If 'setting_names' is passed-in as None : all the settings names and > their corresponding values will be returned as map. > If 'setting_names' is passed-in as empty set : None will be returned. > > - **get_cluster_setting_value(setting_name)** : > -- Retrieves the passed-in cluster setting entry's value. > > - **is_security_enabled()** : > -- Retrieves the cluster's security status. > > > **Functions for stackSettings:** > **----------------------------** > > Stack settings as of now has 5 settings : stack_name, stack_root, > stack_features, stack_tools, stack_packages. stack_name, stack_root have > string as values, whereas stack_features, stack_tools, stack_packages have > values as JSON. Further there already exists python functions in files : > **stack_features.py**, **stack_tools.py** and **stack_select.py**. > > - **get_stack_setting_entries(setting_names)** : > -- Retrieves the passed-in stack setting entr(y/ies) and their values > as a map. > If 'setting_names' is passed-in as None, all the settings names > and their corresponding values will be returned as map. > If 'setting_names' is passed-in as empty set : None will be > returned. > > - **get_stack_setting_value(setting_name)**: > -- Retrieves the passed-in stack setting entry's value. > > - **get_stack_name()**: > -- Retrieves the stack name. > > - **get_stack_root()**: > -- Retrieves the stack root. > > > > **Modifications in _stack_features.py, stack_tools.py and stack_select.py_ > files:** > **-----------------------------------------------------------------------------** > > - Given that these already exist and as of now they read the relevant stack > setting from *configurations/cluster_env*. > - Thus, code has been added to try reading from /stackSettings first by > calling the new fn.() get_stack_setting_value(). if setting not found, go for > the fall back *configurations/cluster_env* (which would be removed soon, > when we remove cluster_env). > > > Diffs > ----- > > > ambari-common/src/main/python/resource_management/libraries/functions/cluster_settings.py > PRE-CREATION > > ambari-common/src/main/python/resource_management/libraries/functions/stack_features.py > 92823b0 > > ambari-common/src/main/python/resource_management/libraries/functions/stack_select.py > b741a33 > > ambari-common/src/main/python/resource_management/libraries/functions/stack_settings.py > PRE-CREATION > > ambari-common/src/main/python/resource_management/libraries/functions/stack_tools.py > d9233a3 > > > Diff: https://reviews.apache.org/r/64594/diff/1/ > > > Testing > ------- > > Python UT passes. > > > **Testing:** > > Tested on live cluster > > > **=================** > **clusterSettings:** > **=================** > > **A.** get_cluster_setting_entries(): > **----------------------------------** > > - 1. Retrieve **single** setting : 'recovery_enabled' > -- In get_cluster_setting_entries(). Passed-in setting(s) : > set(['recovery_enabled']) > **o/p**: {'recovery_enabled': True} > > - 2. Retrieve **two** settings : 'recovery_enabled', > 'sysprep_skip_setup_jce' > -- In get_cluster_setting_entries(). Passed-in setting(s) : > set(['recovery_enabled', 'sysprep_skip_setup_jce']) > **o/p**: {'recovery_enabled': True, 'sysprep_skip_setup_jce': False} > > > - 3. Retrieve settings where passed in empty set -> Expected nothing returned > -- In get_cluster_setting_entries(). Passed-in setting(s) : set([]) > **o/p**: None > > > - 4. Retrieve **three** settings : 'smokeuser', 'recovery_enabled', > 'sysprep_skip_setup_jce' > -- In get_cluster_setting_entries(). Passed-in setting(s) : > set(['smokeuser', 'recovery_enabled', 'sysprep_skip_setup_jce']) > **o/p**: {'recovery_enabled': True, 'sysprep_skip_setup_jce': False, > 'smokeuser': 'ambari-qa'} > > > - 5. Retrieve **three** settings where **middle setting is non-existent** > -- In get_cluster_setting_entries(). Passed-in setting(s) : > set(['recovery_enabled', 'abc', 'sysprep_skip_setup_jce']) > **o/p** : {'recovery_enabled': True, 'sysprep_skip_setup_jce': False} > > > - 6. Retrieve **three** settings where **1st setting is non-existent** > -- In get_cluster_setting_entries(). Passed-in setting(s) : set(['abc', > 'recovery_enabled', 'sysprep_skip_setup_jce']) > **o/p** : {'recovery_enabled': True, 'sysprep_skip_setup_jce': False} > > > - 7. Retrieve **three** settings where **last setting is non-existent** > -- In get_cluster_setting_entries(). Passed-in setting(s) : > set(['recovery_enabled', 'sysprep_skip_setup_jce', 'abc']) > **o/p**: {'recovery_enabled': True, 'sysprep_skip_setup_jce': False} > > > - 8. Retrieve passed in setting which is **non-existent** > -- In get_cluster_setting_entries(). Passed-in setting(s) : > set(['non-existent1']) > **o/p** : None > > > - 9. Retrieve **two** passed in settings and both are non-existent > -- In get_cluster_setting_entries(). Passed-in setting(s) : > set(['non-existent1', 'non-existent2']) > **o/p** : None > > > - 10. Retrieve settings where set passed in is None. -> **returns all > settings.** > -- In get_cluster_setting_entries(). Passed-in setting(s) : None > **o/p**: {'security_enabled': 'false', > 'namenode_rolling_restart_timeout': '4200', 'enable_external_ranger': > 'false', 'override_uid': 'true', 'kerberos_domain': 'EXAMPLE.COM', > 'one_dir_per_partition': 'false', 'agent_mounts_ignore_list': '', > 'repo_ubuntu_template': '{{package_type}} {{base_url}} {{components}}', > 'ignore_groupsusers_create': 'false', 'alerts_repeat_tolerance': '1', > 'hide_yarn_memory_widget': 'false', 'fetch_nonlocal_groups': 'true', > 'manage_dirs_on_root': 'true', 'recovery_lifetime_max_count': '1024', > 'recovery_type': 'AUTO_START', 'ignore_bad_mounts': 'false', > 'recovery_window_in_minutes': '60', 'sysprep_skip_copy_tarballs_hdfs': > 'false', 'user_group': 'hadoop', > 'namenode_rolling_restart_safemode_exit_timeout': '3600', > 'recovery_retry_interval': '5', 'sysprep_skip_copy_oozie_share_lib_to_hdfs': > 'false', 'sysprep_skip_setup_jce': 'false', 'manage_hive_fsroot': 'true', > 'service_check_type': 'full', 'recovery_enabled': 'true', > 'recovery_max_count': '6', 's ysprep_skip_create_users_and_groups': 'false', 'smokeuser_keytab': '/etc/security/keytabs/smokeuser.headless.keytab', 'managed_hdfs_resource_property_names': 'false', 'smokeuser': 'ambari-qa', 'sysprep_skip_copy_fast_jar_hdfs': 'false'} > > > **B.** get_cluster_setting_value(): > **--------------------------------** > > - 1. Retrieve cluster_setting : 'hide_yarn_memory_widget' > -- In get_cluster_setting_value(). Passed-in setting : > hide_yarn_memory_widget > **o/p** : false > > - 2. Retrieve cluster_setting : 'recovery_max_count' > -- In get_cluster_setting_value(). Passed-in setting : > recovery_max_count > **o/p** : 6 > > - 3. Retrieve cluster_setting where passed in setting is 'non_existing' > --> Empty string > -- In get_cluster_setting_value(). Passed-in setting : non_existing > **o/p** : None > > - 4. Retrieve cluster_setting setting passed in is "None" > -- In get_cluster_setting_value(). Passed-in setting : None > **o/p** : None > > > **C**. is_security_enabled(): > **-------------------------** > > - 1. Retrieve security state of cluster by calling > 'is_security_enabled()' which in turn calls **get_cluster_setting_value()** > -- In get_cluster_setting_value(). Passed-in setting : > security_enabled > **o/p** : false > > > **=================** > **stackSettings**: > **=================** > > > **A.** Tests for **get_stack_setting_entries()** > **--------------------------------------------** > > - 1. Retrieve : **stack_name** > > -- In get_stack_setting_entries(). Passed-in setting(s) : > set(['stack_name']) > o/p: {'stack_name': 'HDP'} > > - 2. Retrieve : **stack_root** > -- In get_stack_setting_entries(). Passed-in setting(s) : > set(['stack_root']) > **o/p**: {'stack_root': '{"HDP":"/usr/hdp"}'} > > > - 3. Retrieve **three** stack settings: **'stack_name', 'stack_root', > 'stack_tools'** > -- In get_stack_setting_entries(). Passed-in setting(s) : > set(['stack_name', 'stack_root', 'stack_tools']) > **o/p** : {'stack_name': 'HDP', 'stack_root': '{"HDP":"/usr/hdp"}', > 'stack_tools': '{\n "HDP": {\n "stack_selector": [\n "hdp-select",\n > "/usr/bin/hdp-select",\n "hdp-select"\n ],\n "conf_selector": > [\n "conf-select",\n "/usr/bin/conf-select",\n "conf-select"\n > ]\n }\n}'} > > - 4. Retrieve **four** stack settings : **'BAD_VALUE', 'stack_name', > 'stack_root', 'stack_tools'** ---> (**BAD_VALUE** should get ignored) > -- In get_stack_setting_entries(). Passed-in setting(s) : > set(['stack_name', 'stack_root', 'stack_tools']) > **o/p** : {'stack_name': 'HDP', 'stack_root': '{"HDP":"/usr/hdp"}', > 'stack_tools': '{\n "HDP": {\n "stack_selector": [\n "hdp-select",\n > "/usr/bin/hdp-select",\n "hdp-select"\n ],\n "conf_selector": > [\n "conf-select",\n "/usr/bin/conf-select",\n "conf-select"\n > ]\n }\n}'} > > > **B.** Tests for existing and modified : **stack_select.get_packages()** > which internally now calls **stack_settings.get_stack_setting_value()** > **------------------------------------------------------------------------------------------** > > - 1. Retrieve **specific component package** with call -> > stack_select.get_packages(orchestration, "HDFS", "DATANODE") > -- In get_stack_setting_value(). Passed-in setting : stack_packages > **o/p** : ['hadoop-hdfs-datanode'] > > - 2. Retrieve **specific component package** with call -> > stack_select.get_packages(orchestration, "ZOOKEEPER", "ZOOKEEPER_CLIENT") > -- In get_stack_setting_value(). Passed-in setting : stack_packages > **o/p** : ['zookeeper-client'] > > - 3. Retrieve **non-existing component package** with call -> > stack_select.get_packages(orchestration, "ZOOKEEPER", "NON_EXISTING") > -- In get_stack_setting_value(). Passed-in setting : stack_packages > Skipping stack-select on ABC because it does not exist in the > stack-select package structure. > **o/p** : None > > > **C**. Tests for existing and modified : > **stack_features.check_stack_feature()** which internally now calls > **stack_settings.get_stack_setting_value()** > **------------------------------------------------------------------------------------------** > > - 1. Retrieve **specific component stack feature** with call -> > stack_features.check_stack_feature("hive_server_interactive", "2.6") > -- In get_stack_setting_value(). Passed-in setting : stack_features > **o/p** : True > > - 2. Retrieve **specific component stack feature** with call -> > stack_features.check_stack_feature("hive_server_interactive", "2.2") > -- In get_stack_setting_value(). Passed-in setting : stack_features > **o/p** : False > > **D**. Test for existing and modified : **stack_tools.get_stack_tool()** > which internally now calls **stack_settings.get_stack_setting_value()** > **-------------------------------------------------------------------------------------------** > > - 1. Retrieve **stack relevant tools** with call -> > stack_tools.get_stack_tool("stack_selector") > -- In get_stack_setting_value(). Passed-in setting : stack_tools > **o/p**: ('hdp-select', '/usr/bin/hdp-select', 'hdp-select') > > > Thanks, > > Swapan Shridhar > >
