You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@ambari.apache.org by Jonathan Hurley <jh...@hortonworks.com> on 2017/09/19 17:38:08 UTC
Review Request 62414: Always Take Target Read-Only Properties On Stack
Upgrade
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62414/
-----------------------------------------------------------
Review request for Ambari, Nate Cole and Robert Levas.
Bugs: AMBARI-21999
https://issues.apache.org/jira/browse/AMBARI-21999
Repository: ambari
Description
-------
STR:
- Install a stack, such as HDP 2.3
- Modify the {{cluster-env/stack_features}} structure
- Upgrade to HDP 2.6
The auto-merge which happens can't properly merge {{cluster-env/stack_features}} since it has deviated from the stock values of HDP 2.3. This is dangerous since this is needed for proper operation.
Because it's defined as a read-only value, we should be able to detect this in the auto-merge fro the upgrade and force it to the new stack's default value...
Diffs
-----
ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java 9aa9e31a21
Diff: https://reviews.apache.org/r/62414/diff/1/
Testing
-------
PENDING...
Thanks,
Jonathan Hurley
Re: Review Request 62414: Always Take Target Read-Only Properties On
Stack Upgrade
Posted by Robert Levas <rl...@hortonworks.com>.
> On Sept. 19, 2017, 2:32 p.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java
> > Lines 1176 (patched)
> > <https://reviews.apache.org/r/62414/diff/1/?file=1829942#file1829942line1179>
> >
> > Unless this would be used for something else, wouldn't it be simpler if this method returned a boolean value rather than a Set? You could then potentially save some time by not having to visit all of the properties
>
> Jonathan Hurley wrote:
> We make this call once before traversing each service's properties. This way, we have all of the read-only properties for that service ahead of time. If we returned a boolean, wouldn't that mean that we'd need to iterate the services properties more than once?
Yikes.. you are correct. I am not sure how I missed that. Sorry about about.
- Robert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62414/#review185717
-----------------------------------------------------------
On Sept. 19, 2017, 1:38 p.m., Jonathan Hurley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62414/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2017, 1:38 p.m.)
>
>
> Review request for Ambari, Nate Cole and Robert Levas.
>
>
> Bugs: AMBARI-21999
> https://issues.apache.org/jira/browse/AMBARI-21999
>
>
> Repository: ambari
>
>
> Description
> -------
>
> STR:
> - Install a stack, such as HDP 2.3
> - Modify the {{cluster-env/stack_features}} structure
> - Upgrade to HDP 2.6
>
> The auto-merge which happens can't properly merge {{cluster-env/stack_features}} since it has deviated from the stock values of HDP 2.3. This is dangerous since this is needed for proper operation.
>
> Because it's defined as a read-only value, we should be able to detect this in the auto-merge fro the upgrade and force it to the new stack's default value...
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java 9aa9e31a21
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackUpgradeConfigurationMergeTest.java 474eb50562
>
>
> Diff: https://reviews.apache.org/r/62414/diff/2/
>
>
> Testing
> -------
>
> [INFO] Tests run: 4883, Failures: 0, Errors: 0, Skipped: 34
> [INFO]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 24:34 min
> [INFO] Finished at: 2017-09-19T14:04:35-04:00
> [INFO] Final Memory: 61M/898M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Jonathan Hurley
>
>
Re: Review Request 62414: Always Take Target Read-Only Properties On
Stack Upgrade
Posted by Jonathan Hurley <jh...@hortonworks.com>.
> On Sept. 19, 2017, 2:32 p.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java
> > Lines 1091-1092 (original), 1095-1099 (patched)
> > <https://reviews.apache.org/r/62414/diff/1/?file=1829942#file1829942line1097>
> >
> > Only if you are making other changes, this can be simplified:
> >
> > ```
> > Set<String> readOnlyPropertiesForType = readOnlyProperties.get(configurationType);
> > boolean readOnly = (null != readOnlyPropertiesForType && readOnlyPropertiesForType.contains(existingConfigurationKey));
> > ```
Sure, will do ...
> On Sept. 19, 2017, 2:32 p.m., Robert Levas wrote:
> > ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java
> > Lines 1176 (patched)
> > <https://reviews.apache.org/r/62414/diff/1/?file=1829942#file1829942line1179>
> >
> > Unless this would be used for something else, wouldn't it be simpler if this method returned a boolean value rather than a Set? You could then potentially save some time by not having to visit all of the properties
We make this call once before traversing each service's properties. This way, we have all of the read-only properties for that service ahead of time. If we returned a boolean, wouldn't that mean that we'd need to iterate the services properties more than once?
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62414/#review185717
-----------------------------------------------------------
On Sept. 19, 2017, 1:38 p.m., Jonathan Hurley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62414/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2017, 1:38 p.m.)
>
>
> Review request for Ambari, Nate Cole and Robert Levas.
>
>
> Bugs: AMBARI-21999
> https://issues.apache.org/jira/browse/AMBARI-21999
>
>
> Repository: ambari
>
>
> Description
> -------
>
> STR:
> - Install a stack, such as HDP 2.3
> - Modify the {{cluster-env/stack_features}} structure
> - Upgrade to HDP 2.6
>
> The auto-merge which happens can't properly merge {{cluster-env/stack_features}} since it has deviated from the stock values of HDP 2.3. This is dangerous since this is needed for proper operation.
>
> Because it's defined as a read-only value, we should be able to detect this in the auto-merge fro the upgrade and force it to the new stack's default value...
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java 9aa9e31a21
>
>
> Diff: https://reviews.apache.org/r/62414/diff/1/
>
>
> Testing
> -------
>
> PENDING...
>
>
> Thanks,
>
> Jonathan Hurley
>
>
Re: Review Request 62414: Always Take Target Read-Only Properties On
Stack Upgrade
Posted by Robert Levas <rl...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62414/#review185717
-----------------------------------------------------------
Ship it!
Ship It!
ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java
Lines 1091-1092 (original), 1095-1099 (patched)
<https://reviews.apache.org/r/62414/#comment262027>
Only if you are making other changes, this can be simplified:
```
Set<String> readOnlyPropertiesForType = readOnlyProperties.get(configurationType);
boolean readOnly = (null != readOnlyPropertiesForType && readOnlyPropertiesForType.contains(existingConfigurationKey));
```
ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java
Lines 1176 (patched)
<https://reviews.apache.org/r/62414/#comment262028>
Unless this would be used for something else, wouldn't it be simpler if this method returned a boolean value rather than a Set? You could then potentially save some time by not having to visit all of the properties
- Robert Levas
On Sept. 19, 2017, 1:38 p.m., Jonathan Hurley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62414/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2017, 1:38 p.m.)
>
>
> Review request for Ambari, Nate Cole and Robert Levas.
>
>
> Bugs: AMBARI-21999
> https://issues.apache.org/jira/browse/AMBARI-21999
>
>
> Repository: ambari
>
>
> Description
> -------
>
> STR:
> - Install a stack, such as HDP 2.3
> - Modify the {{cluster-env/stack_features}} structure
> - Upgrade to HDP 2.6
>
> The auto-merge which happens can't properly merge {{cluster-env/stack_features}} since it has deviated from the stock values of HDP 2.3. This is dangerous since this is needed for proper operation.
>
> Because it's defined as a read-only value, we should be able to detect this in the auto-merge fro the upgrade and force it to the new stack's default value...
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java 9aa9e31a21
>
>
> Diff: https://reviews.apache.org/r/62414/diff/1/
>
>
> Testing
> -------
>
> PENDING...
>
>
> Thanks,
>
> Jonathan Hurley
>
>
Re: Review Request 62414: Always Take Target Read-Only Properties On
Stack Upgrade
Posted by Nate Cole <nc...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62414/#review185754
-----------------------------------------------------------
Ship it!
ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java
Lines 1194-1201 (patched)
<https://reviews.apache.org/r/62414/#comment262108>
nit: formatting
- Nate Cole
On Sept. 19, 2017, 1:38 p.m., Jonathan Hurley wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62414/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2017, 1:38 p.m.)
>
>
> Review request for Ambari, Nate Cole and Robert Levas.
>
>
> Bugs: AMBARI-21999
> https://issues.apache.org/jira/browse/AMBARI-21999
>
>
> Repository: ambari
>
>
> Description
> -------
>
> STR:
> - Install a stack, such as HDP 2.3
> - Modify the {{cluster-env/stack_features}} structure
> - Upgrade to HDP 2.6
>
> The auto-merge which happens can't properly merge {{cluster-env/stack_features}} since it has deviated from the stock values of HDP 2.3. This is dangerous since this is needed for proper operation.
>
> Because it's defined as a read-only value, we should be able to detect this in the auto-merge fro the upgrade and force it to the new stack's default value...
>
>
> Diffs
> -----
>
> ambari-server/src/main/java/org/apache/ambari/server/state/UpgradeHelper.java 9aa9e31a21
> ambari-server/src/test/java/org/apache/ambari/server/controller/internal/StackUpgradeConfigurationMergeTest.java 474eb50562
>
>
> Diff: https://reviews.apache.org/r/62414/diff/2/
>
>
> Testing
> -------
>
> [INFO] Tests run: 4883, Failures: 0, Errors: 0, Skipped: 34
> [INFO]
> [INFO] ------------------------------------------------------------------------
> [INFO] BUILD SUCCESS
> [INFO] ------------------------------------------------------------------------
> [INFO] Total time: 24:34 min
> [INFO] Finished at: 2017-09-19T14:04:35-04:00
> [INFO] Final Memory: 61M/898M
> [INFO] ------------------------------------------------------------------------
>
>
> Thanks,
>
> Jonathan Hurley
>
>