> On Nov. 16, 2016, 10:24 a.m., Robert Levas 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> > > > > If this is left as-is, wouldn't it be more efficient to wait to call > > `clusterDAO.findConfig(configId)` in the event we already have the > > properties data and therefore we would not need the entity?
Sure would! It's a JPA cache hit since it's a find by PK, but still - I can move the call. What are your thoughts about doing this lazily still? Should we leave it as-is, or move it into the constructor since this business object instance is stored in memory? > On Nov. 16, 2016, 10:24 a.m., Robert Levas wrote: > > ambari-server/src/main/java/org/apache/ambari/server/state/ConfigImpl.java, > > lines 313-314 > > <https://reviews.apache.org/r/53809/diff/1/?file=1565165#file1565165line313> > > > > Similar issue as above. Yep - same as above. I'll correct it. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53809/#review156067 ----------------------------------------------------------- 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 > >
