You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ambari.apache.org by Andriy Babiichuk <ab...@hortonworks.com> on 2015/05/18 16:49:31 UTC
Review Request 34356: Refactor service-config 'defaultValue' property
into 'savedValue' and 'recommendedValue'
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34356/
-----------------------------------------------------------
Review request for Ambari and Oleg Nechiporenko.
Bugs: AMBARI-11216
https://issues.apache.org/jira/browse/AMBARI-11216
Repository: ambari
Description
-------
{{App.ServiceConfigProperty}} class has this property called {{defaultValue}} which is overloaded and confusing. We want to refactor it into distinct properties called {{savedValue}} and {{recommendedValue}}.
*Background*
Originally this property represented the default value provided by the {{/api/v1/stacks}} endpoint so that when installing cluster the {{value}} could be accurately populated. Later to support the _Undo_ action we put the values saved in {{/api/v1/clusters/c1/configurations}} into the same {{defaultValue}} property. So if {{/api/v1/stacks}} had value '1', and user saved '2', then {{defaultValue}} would become '2'. If ever {{value}} differed from {{defaultValue}}, then _Undo_ action and other logics would trigger.
*Problem*
With the increasing usage of {{/recommendations}} endpoint for config's dynamic values and value-attributes, the {{defaultValue}} is getting complex to understand and comprehend. We generally tend to update {{defaultValue}} whenever {{value}} is updated - which is hard to explain. For example in the add-service wizard we [suppress any updates to core-site.xml|https://github.com/apache/ambari/blob/trunk/ambari-web/app/controllers/wizard/step8_controller.js#L864] because we cannot tell when core-site.xml configs were changed from what is saved on server.
*Proposal*
Each config should have 3 values, as they are 3 different entities:
# {{value}} - Represents the value shown in UI only to the user
# {{savedValue}} - Represents the value saved in Ambari DB in {{/api/v1/clusters/c1/configurations}}. Should only be set when loading configs from API.
# {{recommendedValue}} - Represents the value either provided by {{/api/v1/stacks}} or by {{/recommendations}}.
This will cleanly differentiate the purpose of each variable. This will have an effect on the logic used like {{#isNotDefaultValue}} and {{#isOverrideChanged}} etc.
Undo action should be whenever {{value}} != {{savedValue}}.
Set recommended action should be whenever {{value}} != {{recommendedValue}}
Add-service wizard should basically save only those config-types which have {{value}} != {{savedValue}} etc.
The change might be as simple as renaming {{defaultValue}} to {{savedValue}}, but the semantics will be clearer to understand and code.
Diffs
-----
ambari-web/app/controllers/main/admin/highAvailability/nameNode/step3_controller.js 83b3ee704082692da05f6bd8689fe4040160b85c
ambari-web/app/controllers/main/admin/highAvailability/resourceManager/step3_controller.js 7a2e250b84e12ee4248cd18207f12c006d61c73f
ambari-web/app/controllers/main/admin/kerberos.js 702d585faac40951d8720293dd6c18ebec6d9255
ambari-web/app/controllers/main/admin/kerberos/step4_controller.js 47a41078b4094374a12e6fd8bb10e54722d6c9fd
ambari-web/app/controllers/main/service/info/configs.js abd7048f7435492ea30a18db2f325f95f3dafbb3
ambari-web/app/controllers/wizard.js 22d3bbcd4773fb886794a926330f3cb5d2db481a
ambari-web/app/controllers/wizard/step7_controller.js bc716b510fa3f505f006e64de039d259f4e5b9d9
ambari-web/app/controllers/wizard/step8_controller.js 256972368dfc0ae99afef96cd40f12d3400f041a
ambari-web/app/data/BIGTOP/site_properties.js a8109b5c905bcbaf55d3dae847907431c3d2a6d8
ambari-web/app/data/HDP2.2/site_properties.js 41861a7e2ec5b3f87baeb52669c98611fce0c40b
ambari-web/app/data/HDP2.3/site_properties.js e46d600dc747e5cc813ca4c2b4f28c054a51e53b
ambari-web/app/data/HDP2/custom_configs.js ba1376b77e32c593a484583fd0560f843d9fc643
ambari-web/app/data/HDP2/ha_properties.js 6599f77cd85727402b74618b83f0455f3d9e8f8d
ambari-web/app/data/HDP2/rm_ha_properties.js ef77b1e81d8ebc8710385893190ff0f8efca6a3d
ambari-web/app/data/HDP2/secure_properties.js 2bdb4baee2e10aeae15b5a4a90e5b4673c866afe
ambari-web/app/data/HDP2/site_properties.js e818799478195e00dfddd1a2dfb5b71097393cca
ambari-web/app/data/PHD/site_properties.js 9a2aa4b62210a2fa9177c233e95e591a8edfb506
ambari-web/app/mappers/configs/config_versions_mapper.js aad5a3b2f683ccf220eff4eb0a94a4f9aea23ca8
ambari-web/app/mappers/configs/stack_config_properties_mapper.js 3d24df8be42c68a4c3370b4365590d5026a0a54c
ambari-web/app/messages.js a93ab1f6ab4cf21a09384c00c8c01aad991ffab2
ambari-web/app/mixins/common/configs/configs_saver.js 944133bd5688f2edfff92c7d1eff29a243078561
ambari-web/app/mixins/common/configs/enhanced_configs.js 853a1abc4231b6964a237285f97c922a9421ae98
ambari-web/app/models/configs/config_property.js facaaf414bfc657d6909384d485c549f00d2eb3b
ambari-web/app/models/configs/objects/service_config.js fe4c8f0d1d526d2ad8e91565c5aeff91ab7d30d2
ambari-web/app/models/configs/objects/service_config_property.js c8971ec299f07b469100b57dfb3bf6abbd1d6353
ambari-web/app/models/configs/stack_config_property.js 5f660497ec929deec9fce2e0522c280c5f6291b6
ambari-web/app/templates/common/modal_popups/dependent_configs_list.hbs c42d50d7120d0a0f8bcb5808e9952f3a11c07a2d
ambari-web/app/utils/config.js 518405160de2922a0ad5cdbdaa6d4e83e420dbe8
ambari-web/app/utils/configs/config_property_helper.js 07226c58cc480e38f589d8b275607b04bc865eaf
ambari-web/app/views.js 6af5d2f386efae6e8dcd51080903b931f82f03e9
ambari-web/app/views/common/configs/service_configs_by_category_view.js 5cfc3e90e31fe5ad232d0d9c14deaf7ee057ffa1
ambari-web/app/views/common/configs/widgets/checkbox_config_widget_view.js 03d3d3fbe4cff5a2d698a5f85f849e80ee59526c
ambari-web/app/views/common/configs/widgets/combo_config_widget_view.js a9346ab7b426ea26e1ea29c0162417c04a322661
ambari-web/app/views/common/configs/widgets/config_widget_view.js 0b3939cc6fa07f5baec1f71e5e08a6f2e259b882
ambari-web/app/views/common/configs/widgets/directory_config_widget_view.js be589b016565c12fee8f31b9377b5e18c27a4868
ambari-web/app/views/common/configs/widgets/list_config_widget_view.js 149f87de1da5125b42c18ba1ad27b25f561915aa
ambari-web/app/views/common/configs/widgets/plain_config_text_field.js 27d6b3bc57a83376af223b0e7192c4ac8e7db49c
ambari-web/app/views/common/configs/widgets/slider_config_widget_view.js e618020b75714b1b9b0d2e333d6a979260e4c624
ambari-web/app/views/common/configs/widgets/time_interval_spinner_view.js fb5879dbac72992302b81eb04cec11c1d58f4304
ambari-web/app/views/common/configs/widgets/toggle_config_widget_view.js 8aec220edc98b0bf6839f1c82da530329235eab3
ambari-web/app/views/common/controls_view.js 6dd82b7bf7a8cba845091cac9eaa9304a08f5981
ambari-web/app/views/common/modal_popups/prompt_popup.js df454890240ba26010498d83fbee5abdfd936b7f
ambari-web/test/controllers/main/admin/highAvailability/resourceManager/step3_controller_test.js 21416d62d0cd1d7620a5005853135b2920b21411
ambari-web/test/controllers/main/admin/kerberos/step4_controller_test.js b45a3a485479e5dde799bfbbfb3a70ccafc1eb92
ambari-web/test/controllers/main/service/info/config_test.js 7d46d9d12fb6e4811971772db88b3e9f71d496e3
ambari-web/test/controllers/wizard/step7_test.js 044ebbb6315a6bde4edd0fe8ef3f224bdc80289b
ambari-web/test/controllers/wizard/step8_test.js 432f8e0196fb9e8417bba30240ab0d8856d14b56
ambari-web/test/controllers/wizard_test.js 9b9c06f9b66cd3bdde08a35d257de37d1e8bff11
ambari-web/test/data/HDP2/site_properties_test.js 0336f923a8342b7f709bf5d57befd34fff598faa
ambari-web/test/mappers/configs/config_versions_mapper_test.js 4e590fee660cf6d08745fb22c2efe6290da46ee7
ambari-web/test/mappers/configs/stack_config_properties_mapper_test.js 416a14fca637eef572d07b7a05fa5ac3bd936159
ambari-web/test/mock_data_setup/configs_mock_data.js 0136f9b10f0e0ab04a668e5fa71386bd1951d006
ambari-web/test/models/configs/config_property_test.js 84cae0b000086240c3fcdffe753a0a15fb26a20a
ambari-web/test/models/configs/objects/service_config_property_test.js a71e838204a7f41b1dca97e224d289eb9c6fd613
ambari-web/test/utils/config_test.js ccc693186718036ca5ded5ac4a91e41c27e11646
ambari-web/test/utils/configs/config_property_helper_test.js d66f4022de4b3be1481f48fd52f8fa4d274f1c18
ambari-web/test/views/common/configs/service_configs_by_category_view_test.js 7c14dac218324774379f9ac7a0d6c2fcb7563871
ambari-web/test/views/common/configs/widgets/list_config_widget_view_test.js 3e301d68854b27e420f11fb7b5f98d0b6506e546
ambari-web/test/views/common/configs/widgets/slider_config_widget_view_test.js fba4493b84073e19dfdc61230baeb3ffaec4860c
ambari-web/test/views/common/configs/widgets/toggle_config_widget_view_test.js 76e033026f3b5dd7b29152937edfe8d73c2db076
ambari-web/test/views/common/controls_view_test.js 463edb9302246d99d084726987c5e0d9c87a78fb
Diff: https://reviews.apache.org/r/34356/diff/
Testing
-------
6116 tests complete (10 seconds)
86 tests pending
Thanks,
Andriy Babiichuk
Re: Review Request 34356: Refactor service-config 'defaultValue'
property into 'savedValue' and 'recommendedValue'
Posted by Oleg Nechiporenko <on...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34356/#review84155
-----------------------------------------------------------
Ship it!
Ship It!
- Oleg Nechiporenko
On May 18, 2015, 2:49 p.m., Andriy Babiichuk wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34356/
> -----------------------------------------------------------
>
> (Updated May 18, 2015, 2:49 p.m.)
>
>
> Review request for Ambari and Oleg Nechiporenko.
>
>
> Bugs: AMBARI-11216
> https://issues.apache.org/jira/browse/AMBARI-11216
>
>
> Repository: ambari
>
>
> Description
> -------
>
> {{App.ServiceConfigProperty}} class has this property called {{defaultValue}} which is overloaded and confusing. We want to refactor it into distinct properties called {{savedValue}} and {{recommendedValue}}.
>
> *Background*
> Originally this property represented the default value provided by the {{/api/v1/stacks}} endpoint so that when installing cluster the {{value}} could be accurately populated. Later to support the _Undo_ action we put the values saved in {{/api/v1/clusters/c1/configurations}} into the same {{defaultValue}} property. So if {{/api/v1/stacks}} had value '1', and user saved '2', then {{defaultValue}} would become '2'. If ever {{value}} differed from {{defaultValue}}, then _Undo_ action and other logics would trigger.
>
> *Problem*
> With the increasing usage of {{/recommendations}} endpoint for config's dynamic values and value-attributes, the {{defaultValue}} is getting complex to understand and comprehend. We generally tend to update {{defaultValue}} whenever {{value}} is updated - which is hard to explain. For example in the add-service wizard we [suppress any updates to core-site.xml|https://github.com/apache/ambari/blob/trunk/ambari-web/app/controllers/wizard/step8_controller.js#L864] because we cannot tell when core-site.xml configs were changed from what is saved on server.
>
> *Proposal*
> Each config should have 3 values, as they are 3 different entities:
> # {{value}} - Represents the value shown in UI only to the user
> # {{savedValue}} - Represents the value saved in Ambari DB in {{/api/v1/clusters/c1/configurations}}. Should only be set when loading configs from API.
> # {{recommendedValue}} - Represents the value either provided by {{/api/v1/stacks}} or by {{/recommendations}}.
>
> This will cleanly differentiate the purpose of each variable. This will have an effect on the logic used like {{#isNotDefaultValue}} and {{#isOverrideChanged}} etc.
>
> Undo action should be whenever {{value}} != {{savedValue}}.
> Set recommended action should be whenever {{value}} != {{recommendedValue}}
>
> Add-service wizard should basically save only those config-types which have {{value}} != {{savedValue}} etc.
>
>
> The change might be as simple as renaming {{defaultValue}} to {{savedValue}}, but the semantics will be clearer to understand and code.
>
>
> Diffs
> -----
>
> ambari-web/app/controllers/main/admin/highAvailability/nameNode/step3_controller.js 83b3ee704082692da05f6bd8689fe4040160b85c
> ambari-web/app/controllers/main/admin/highAvailability/resourceManager/step3_controller.js 7a2e250b84e12ee4248cd18207f12c006d61c73f
> ambari-web/app/controllers/main/admin/kerberos.js 702d585faac40951d8720293dd6c18ebec6d9255
> ambari-web/app/controllers/main/admin/kerberos/step4_controller.js 47a41078b4094374a12e6fd8bb10e54722d6c9fd
> ambari-web/app/controllers/main/service/info/configs.js abd7048f7435492ea30a18db2f325f95f3dafbb3
> ambari-web/app/controllers/wizard.js 22d3bbcd4773fb886794a926330f3cb5d2db481a
> ambari-web/app/controllers/wizard/step7_controller.js bc716b510fa3f505f006e64de039d259f4e5b9d9
> ambari-web/app/controllers/wizard/step8_controller.js 256972368dfc0ae99afef96cd40f12d3400f041a
> ambari-web/app/data/BIGTOP/site_properties.js a8109b5c905bcbaf55d3dae847907431c3d2a6d8
> ambari-web/app/data/HDP2.2/site_properties.js 41861a7e2ec5b3f87baeb52669c98611fce0c40b
> ambari-web/app/data/HDP2.3/site_properties.js e46d600dc747e5cc813ca4c2b4f28c054a51e53b
> ambari-web/app/data/HDP2/custom_configs.js ba1376b77e32c593a484583fd0560f843d9fc643
> ambari-web/app/data/HDP2/ha_properties.js 6599f77cd85727402b74618b83f0455f3d9e8f8d
> ambari-web/app/data/HDP2/rm_ha_properties.js ef77b1e81d8ebc8710385893190ff0f8efca6a3d
> ambari-web/app/data/HDP2/secure_properties.js 2bdb4baee2e10aeae15b5a4a90e5b4673c866afe
> ambari-web/app/data/HDP2/site_properties.js e818799478195e00dfddd1a2dfb5b71097393cca
> ambari-web/app/data/PHD/site_properties.js 9a2aa4b62210a2fa9177c233e95e591a8edfb506
> ambari-web/app/mappers/configs/config_versions_mapper.js aad5a3b2f683ccf220eff4eb0a94a4f9aea23ca8
> ambari-web/app/mappers/configs/stack_config_properties_mapper.js 3d24df8be42c68a4c3370b4365590d5026a0a54c
> ambari-web/app/messages.js a93ab1f6ab4cf21a09384c00c8c01aad991ffab2
> ambari-web/app/mixins/common/configs/configs_saver.js 944133bd5688f2edfff92c7d1eff29a243078561
> ambari-web/app/mixins/common/configs/enhanced_configs.js 853a1abc4231b6964a237285f97c922a9421ae98
> ambari-web/app/models/configs/config_property.js facaaf414bfc657d6909384d485c549f00d2eb3b
> ambari-web/app/models/configs/objects/service_config.js fe4c8f0d1d526d2ad8e91565c5aeff91ab7d30d2
> ambari-web/app/models/configs/objects/service_config_property.js c8971ec299f07b469100b57dfb3bf6abbd1d6353
> ambari-web/app/models/configs/stack_config_property.js 5f660497ec929deec9fce2e0522c280c5f6291b6
> ambari-web/app/templates/common/modal_popups/dependent_configs_list.hbs c42d50d7120d0a0f8bcb5808e9952f3a11c07a2d
> ambari-web/app/utils/config.js 518405160de2922a0ad5cdbdaa6d4e83e420dbe8
> ambari-web/app/utils/configs/config_property_helper.js 07226c58cc480e38f589d8b275607b04bc865eaf
> ambari-web/app/views.js 6af5d2f386efae6e8dcd51080903b931f82f03e9
> ambari-web/app/views/common/configs/service_configs_by_category_view.js 5cfc3e90e31fe5ad232d0d9c14deaf7ee057ffa1
> ambari-web/app/views/common/configs/widgets/checkbox_config_widget_view.js 03d3d3fbe4cff5a2d698a5f85f849e80ee59526c
> ambari-web/app/views/common/configs/widgets/combo_config_widget_view.js a9346ab7b426ea26e1ea29c0162417c04a322661
> ambari-web/app/views/common/configs/widgets/config_widget_view.js 0b3939cc6fa07f5baec1f71e5e08a6f2e259b882
> ambari-web/app/views/common/configs/widgets/directory_config_widget_view.js be589b016565c12fee8f31b9377b5e18c27a4868
> ambari-web/app/views/common/configs/widgets/list_config_widget_view.js 149f87de1da5125b42c18ba1ad27b25f561915aa
> ambari-web/app/views/common/configs/widgets/plain_config_text_field.js 27d6b3bc57a83376af223b0e7192c4ac8e7db49c
> ambari-web/app/views/common/configs/widgets/slider_config_widget_view.js e618020b75714b1b9b0d2e333d6a979260e4c624
> ambari-web/app/views/common/configs/widgets/time_interval_spinner_view.js fb5879dbac72992302b81eb04cec11c1d58f4304
> ambari-web/app/views/common/configs/widgets/toggle_config_widget_view.js 8aec220edc98b0bf6839f1c82da530329235eab3
> ambari-web/app/views/common/controls_view.js 6dd82b7bf7a8cba845091cac9eaa9304a08f5981
> ambari-web/app/views/common/modal_popups/prompt_popup.js df454890240ba26010498d83fbee5abdfd936b7f
> ambari-web/test/controllers/main/admin/highAvailability/resourceManager/step3_controller_test.js 21416d62d0cd1d7620a5005853135b2920b21411
> ambari-web/test/controllers/main/admin/kerberos/step4_controller_test.js b45a3a485479e5dde799bfbbfb3a70ccafc1eb92
> ambari-web/test/controllers/main/service/info/config_test.js 7d46d9d12fb6e4811971772db88b3e9f71d496e3
> ambari-web/test/controllers/wizard/step7_test.js 044ebbb6315a6bde4edd0fe8ef3f224bdc80289b
> ambari-web/test/controllers/wizard/step8_test.js 432f8e0196fb9e8417bba30240ab0d8856d14b56
> ambari-web/test/controllers/wizard_test.js 9b9c06f9b66cd3bdde08a35d257de37d1e8bff11
> ambari-web/test/data/HDP2/site_properties_test.js 0336f923a8342b7f709bf5d57befd34fff598faa
> ambari-web/test/mappers/configs/config_versions_mapper_test.js 4e590fee660cf6d08745fb22c2efe6290da46ee7
> ambari-web/test/mappers/configs/stack_config_properties_mapper_test.js 416a14fca637eef572d07b7a05fa5ac3bd936159
> ambari-web/test/mock_data_setup/configs_mock_data.js 0136f9b10f0e0ab04a668e5fa71386bd1951d006
> ambari-web/test/models/configs/config_property_test.js 84cae0b000086240c3fcdffe753a0a15fb26a20a
> ambari-web/test/models/configs/objects/service_config_property_test.js a71e838204a7f41b1dca97e224d289eb9c6fd613
> ambari-web/test/utils/config_test.js ccc693186718036ca5ded5ac4a91e41c27e11646
> ambari-web/test/utils/configs/config_property_helper_test.js d66f4022de4b3be1481f48fd52f8fa4d274f1c18
> ambari-web/test/views/common/configs/service_configs_by_category_view_test.js 7c14dac218324774379f9ac7a0d6c2fcb7563871
> ambari-web/test/views/common/configs/widgets/list_config_widget_view_test.js 3e301d68854b27e420f11fb7b5f98d0b6506e546
> ambari-web/test/views/common/configs/widgets/slider_config_widget_view_test.js fba4493b84073e19dfdc61230baeb3ffaec4860c
> ambari-web/test/views/common/configs/widgets/toggle_config_widget_view_test.js 76e033026f3b5dd7b29152937edfe8d73c2db076
> ambari-web/test/views/common/controls_view_test.js 463edb9302246d99d084726987c5e0d9c87a78fb
>
> Diff: https://reviews.apache.org/r/34356/diff/
>
>
> Testing
> -------
>
> 6116 tests complete (10 seconds)
> 86 tests pending
>
>
> Thanks,
>
> Andriy Babiichuk
>
>