----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53809/#review156057 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/RangerKerberosConfigCalculation.java (line 90) <https://reviews.apache.org/r/53809/#comment226197> persist(boolean) goes away since we don't track whether it's been persisted anymore. - If you want a new config, you use the factory to create it. - If you want to update an existing config, you call save() on it. ambari-server/src/main/java/org/apache/ambari/server/state/ConfigImpl.java (lines 212 - 240) <https://reviews.apache.org/r/53809/#comment226196> This is one area I'd love feedback on. I left the double-checked lock here, but it's kind of stupid. We have the data on instantiation (whether new or existing). I think this was done to lazily load the properties since we need to de-serialize from JSON. Should we just be doing this on construction and incur the hit then? Or should we really way until the properties are requested via the get()? Since this is a business object stored in a map, I don't think the de-serialization happens too often. - Jonathan Hurley On Nov. 16, 2016, 8:37 a.m., Jonathan Hurley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53809/ > ----------------------------------------------------------- > > (Updated Nov. 16, 2016, 8:37 a.m.) > > > Review request for Ambari, Nate Cole, Robert Levas, Robert Nettleton, and Sid > Wagle. > > > Bugs: AMBARI-18906 > https://issues.apache.org/jira/browse/AMBARI-18906 > > > Repository: ambari > > > Description > ------- > > {{ConfigImpl}} uses internal locks around state which can lead to deadlocks > when this in called in the context of other business objects. In most cases, > this state-full data does not need locks placed around it. > > {{ConfigImpl}} should be changed to: > - No longer store entity information > - No longer require locks around primitive state > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > b04fdd7 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ConfigGroupResourceProvider.java > 96bb8f9 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/ConfigureAction.java > 5459ddb > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/FixLzoCodecPath.java > ffa21ab > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/FixOozieAdminUsers.java > 3a06476 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/HBaseConfigCalculation.java > 7f6d4b1 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/HBaseEnvMaxDirectMemorySizeAction.java > b238bca > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/HiveEnvClasspathAction.java > 0e10160 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/HiveZKQuorumConfigAction.java > 0ade30b > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/OozieConfigCalculation.java > 4da67ca > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/RangerConfigCalculation.java > ff4a20e > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/RangerKerberosConfigCalculation.java > ba0da79 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/SparkShufflePropertyConfig.java > 299a373 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/upgrades/YarnConfigCalculation.java > feefcaf > ambari-server/src/main/java/org/apache/ambari/server/state/Config.java > b35aad9 > > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigFactory.java > eaf68aa > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigImpl.java > 28bcd5f > > ambari-server/src/main/java/org/apache/ambari/server/state/configgroup/ConfigGroupImpl.java > 9917720 > > ambari-server/src/main/java/org/apache/ambari/server/topology/AmbariContext.java > 83f8470 > > ambari-server/src/main/java/org/apache/ambari/server/update/HostUpdateHelper.java > 6a8057c > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapperTest.java > ffca51d > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionSchedulerThreading.java > 90a4421 > > ambari-server/src/test/java/org/apache/ambari/server/agent/HeartbeatTestHelper.java > 43503fa > > ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatMonitor.java > 76ab45c > > Diff: https://reviews.apache.org/r/53809/diff/ > > > Testing > ------- > > The tests are a mess right now - about 280 compile errors. I'm working > through them. However, I don't think that should hold up feedback on the > review (especially since this is being done in a separate branch) > > > Thanks, > > Jonathan Hurley > >
