Review Request 62414: Always Take Target Read-Only Properties On Stack Upgrade

[Jonathan Hurley]

---
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

[Robert Levas]

---
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)


Only if you are making other changes, this can be simplified:

```
Set 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)


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

[Jonathan Hurley]

> 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)
> > 
> >
> > Only if you are making other changes, this can be simplified:
> > 
> > ```
> > Set 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)
> > 
> >
> > 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

[Robert Levas]

> 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)
> > 
> >
> > 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

[Nate Cole]

---
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)


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
> 
>