> On Nov. 16, 2016, 8:43 a.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigImpl.java, > > lines 281-309 > > <https://reviews.apache.org/r/53809/diff/1/?file=1565165#file1565165line281> > > > > 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.
It seems like most of the time if you are getting the configs, you will want to get the properites. So why not deserialze the properties on instantiation. My guess is the penality for calling `clusterDAO.findConfig(configId)` and handling the locking would outweigh the times we deserialize the data during instantiation and not use it. Or maybe I just have high expectations on the performace of `Gson#fromJson`. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53809/#review156057 ----------------------------------------------------------- On Nov. 16, 2016, 8:43 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:43 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 > > One of the biggest changes was made to how configs are created. We used to > use this anti-pattern: > - Create config in-memory from factory (tracking persistence state) > - Set the tag on the config instance > - Persist the config > - Add config to cluster > > That was ridiculous - the factory should handle the creation of the entity > with the required data. In a few spots, I had caught bugs where we had > duplicated a ton of cluster entity logic when adding configs which was > missing some steps. > > > 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 > >
