> On July 20, 2016, 11:17 p.m., Alexandr Antonenko wrote:
> > ambari-web/app/utils/config.js, line 264
> > <https://reviews.apache.org/r/49387/diff/3-4/?file=1445954#file1445954line264>
> >
> >     I do not see FINAL populated anywhere. I think this code will trigger 
> > fatal error. Have you tested this part manually ? 
> >     
> >     Also, there is no need to check attribute[0], you just created it, and 
> > anyway there will be one and more values in that array
> >     
> >     In code : attributes[0][FINAL][index] && attributes[0][FINAL][index] 
> > === "true"
> >     
> >     You can skip cheking attributes[0][FINAL][index], just check 
> > attributes[0][FINAL][index] === "true".
> >     
> >     If property "attributes[0][FINAL][index]" does not exist 
> > attributes[0][FINAL][index] === "true" will not trigger any error

The updated patch "AMBARI-17041-July21-updated.patch" conatins this correction. 
I have kept the original statement with the name of the variable updated to 
"attributes[0][FINAL]".

Thank you!


> On July 20, 2016, 11:17 p.m., Alexandr Antonenko wrote:
> > ambari-web/app/utils/config.js, line 248
> > <https://reviews.apache.org/r/49387/diff/3-4/?file=1445954#file1445954line248>
> >
> >     FYI: this can be also simplified 
> >     
> >     var attributes = [];
> >     ['FINAL', 'PASSWORD', 'USER', 'GROUP', 'TEXT', 
> > 'ADDITIONAL_USER_PROPERTY', 'NOT_MANAGED_HDFS_PATH', 
> > 'VALUE_FROM_PROPERTY_FILE'].forEach(function (propertyName) {
> >        attributes.push(
> >        {
> >         [propertyName] : Em.get(configJSON, 
> > 'properties_attributes.'+propertyName) || {}})
> >     })

Hello Alexandr,
I tried using the optimized statements you've mentioned here. The file attached 
"AMBARI-17041-July21-ES6.patch" contains the patch using this optimized code. 
The build is successful and even the UI behaves as intended after deploying the 
build. However, the ambari-web tests fail for this patch. I get a Syntax Error 
as seen in attachment "ambari_web_failed_to_execute_test" on the Jira. The 
tests didn't give further information on where the error occurred, but I 
figured out that the square brackets used for "propertyName" in the code above, 
belongs to ECMAScript 6 standard. Somehow, the test framework is not able to 
accept this syntax as valid. On removing the square brackets (only for 
testing), the ambari-web tests ran successfully.

I have used a workaround for this issue by using another variable to store the 
json object and then pushing that json variable to "attributes". This code is 
present in "AMBARI-17041-July21-updated.patch" (updated diff). The ambari-web 
tests are successful. Please let me know if I was making any mistake earlier or 
if I could update the patch in some other way.

Thank you!


- Keta


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49387/#review143024
-----------------------------------------------------------


On July 20, 2016, 9:05 p.m., Keta Patel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49387/
> -----------------------------------------------------------
> 
> (Updated July 20, 2016, 9:05 p.m.)
> 
> 
> Review request for Ambari, Alexandr Antonenko, Di Li, Dmitro Lisnichenko, and 
> Jonathan Hurley.
> 
> 
> Bugs: AMBARI-17041
>     https://issues.apache.org/jira/browse/AMBARI-17041
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> Currently, services can define properties in the XML configuration files that 
> is flagged as type password:
>   <property>
>         <name>my.special.password</name>
>         <value></value>
>         <property-type>PASSWORD</property-type>
>         <description>Password to be masked</description>
>     </property> 
> and it will be masked properly in the UI as well as blueprint.
> 
> Custom property should also support this option so that password can be added 
> as custom property and treat accordingly.
> 
> 
> ==========================================
> Proposed Design for the fix:
> ==========================================
> At present only the key-value information of the service properties is stored 
> in the DB ("clusterconfig" table in the "config_data" column). 
> The "config_attributes" column stores only certain attributes like "final" 
> indicating the list of properties set with the Final flag = true. 
> The information about the property-type (i.e PASSWORD, USER, GROUP, 
> ADDITIONAL_USER_PROPERTY, VALUE_FROM_PROPERTY_FILE, NOT_MANAGED_HDFS_PATH, 
> etc) is extracted from the corresponding service's property file (e.g. 
> hive-site.xml, core-site.xml, webhcat-env.xml, etc). These files contain 
> information of the existing properties only. Custom Properties added by 
> ambari user have no provision to store their additional attributes. 
> 
> Since, for this Jira we are concerned with only <property-type> attribute for 
> Custom Properties, we could add an additional field called "Property Type" in 
> the "Add Property" pop-up which shows up on clicking "Add Property ..." in 
> the Custom property section for a service. For now, only 2 options are shown 
> in the drop-down list: NONE and PASSWORD .
> A few sample test properties are created using the new "Add Property" pop-up 
> as can be seen in the following attachments. 
> Attachments: 
> "add_property_pop_up.tiff"
> "custom_property_password_type.tiff"
> "custom_property_regular_type.tiff"
> "custom_properties_after_save.tiff"
> 
> The <property-type> information for these Custom properties is stored in the 
> DB in "clusterconfig" table, "config_attributes" column.
> The schema for "clusterconfig" table can be seen in the attachment:
> "schema_of_clusterconfig_table.tiff"
> 
> The content of the "config_attributes" column with the <property-type> 
> information from the new Custom properties can be seen in the attachment:
> "cluster_config_with_password_type_in_config_attributes_column.tiff"
> 
> 
> Note: The fix so far is performed only for new Custom properties. The 
> <property-type> information for existing properties is extracted from the 
> corresponding property xml files for the service.
> 
> 
> Diffs
> -----
> 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java
>  066acab 
>   
> ambari-server/src/main/java/org/apache/ambari/server/controller/ConfigurationResponse.java
>  eef3474 
>   
> ambari-server/src/main/java/org/apache/ambari/server/utils/SecretReference.java
>  84f3109 
>   ambari-web/app/messages.js 24c9cfa 
>   ambari-web/app/mixins/common/configs/configs_saver.js 43a77ce 
>   ambari-web/app/models/configs/objects/service_config_property.js 844806f 
>   ambari-web/app/templates/common/configs/addPropertyWindow.hbs 659435c 
>   ambari-web/app/utils/config.js 9bb977a 
>   ambari-web/app/views/common/configs/service_configs_by_category_view.js 
> 964e193 
>   ambari-web/test/mixins/common/configs/configs_saver_test.js 4baf303 
> 
> Diff: https://reviews.apache.org/r/49387/diff/
> 
> 
> Testing
> -------
> 
> No new test cases are written in the patch apart from the fix required in 
> "configs_saver_test.js" to avoid test failure of existing tests.
> If the proposed solution is acceptable, I will create a new patch with the 
> necessary new tests.
> 
> The existing ambari-web tests after applying the patch:
> 
>   28977 tests complete (37 seconds)
>   154 tests pending
>   
>   
> With the new latest patch "AMBARI-17041-trunk-July08.patch" the following is 
> result of ambari-web tests:
> 
>   29017 tests complete (48 seconds)
>   154 tests pending
>   
> With the new patch "AMBARI-17041-trunk-July13.patch" the following is the 
> result of ambari-web tests:
> 
>   29021 tests complete (26 seconds)
>   154 tests pending
>   
> **With the new patch "AMBARI-17041-trunk-July20.patch" the following is the 
> result of ambari-web tests:
>   29292 tests complete (42 seconds)
>   154 tests pending
> 
> 
> Thanks,
> 
> Keta Patel
> 
>

Reply via email to