----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33663/#review83030 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertsDAO.java <https://reviews.apache.org/r/33663/#comment133903> I'm very worried about this pattern. If you did actually fix this bug using @RequireSession, it's way too easy to miss for anyone adding new methods. You've also created an interceptor on methods that get called a lot in larger clusters potentially leading to a bottleneck. Looking at AmbariLocalSessionInterceptor, it's opening a brand new unit of work every time this method is invoked and then closing that unit of work. Seems like an anti-pattern to me; we're not really supposed to be doing that on a per-call basis. Instead, it seems like what needs to be done is have the unit of work opened for the thread so that it can do all of its work (1 to n) queries and then close the unit of work when it's done. Basically, what we have here looks analogous to method-level locking when what you really need is a higher-level lock. Any call into these methods from within the context of a request will have the unit of work already. Anything that does not will need to start and end its own unit of work. Although this approach might work, it's not very performant and is still prone to errors when adding new methods. - Jonathan Hurley On May 8, 2015, 10:10 a.m., Dmitro Lisnichenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33663/ > ----------------------------------------------------------- > > (Updated May 8, 2015, 10:10 a.m.) > > > Review request for Ambari, Alejandro Fernandez, Jonathan Hurley, Myroslav > Papirkovskyy, and Nate Cole. > > > Bugs: AMBARI-10818 > https://issues.apache.org/jira/browse/AMBARI-10818 > > > Repository: ambari > > > Description > ------- > > Problem here is that DistributeRepositoriesActionListener is executed in a > separate thread. So we have to use UnitOfWork just like at > org.apache.ambari.server.actionmanager.ActionScheduler#doWork , otherwise > EntityManager cache is not updated on DB updates. I mean that > RepositoryVersion state at DB is INSTALLING, and API shows INSTALLING, but > RepositoryVersion state in DistributeRepositoriesActionListener is still > INSTALLED, and cluster state transition is not performed. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/upgrade/DistributeRepositoriesActionListener.java > 21e3a48 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertDefinitionDAO.java > d838d25 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertDispatchDAO.java > 85973b1 > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertsDAO.java > 9a2be15 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ClusterDAO.java > d3c4bd4 > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/GroupDAO.java > bd5defb > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java > 119737c > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/MemberDAO.java > e831db2 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/PermissionDAO.java > 939c32b > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/PrincipalDAO.java > 7ac4f05 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/PrincipalTypeDAO.java > 046345a > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/PrivilegeDAO.java > 4fda7bc > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/RepositoryVersionDAO.java > d55a00f > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/RequestDAO.java > 7e190a3 > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/StackDAO.java > 6405068 > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/StageDAO.java > c2b551b > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/TopologyHostTaskDAO.java > 031601a > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/UpgradeDAO.java > a9b913f > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/UserDAO.java > 2fcb087 > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ViewDAO.java > bbbab63 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/ViewInstanceDAO.java > 91a2e72 > > ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatHandler.java > 5be6e44 > > Diff: https://reviews.apache.org/r/33663/diff/ > > > Testing > ------- > > Same tests are failing on trunk > > Tests in error: > > test220Cardinality(org.apache.ambari.server.api.services.KerberosServiceMetaInfoTest): > Guice provision errors:(..) > > test220AutoDeploy(org.apache.ambari.server.api.services.KerberosServiceMetaInfoTest): > Guice provision errors:(..) > > test220Dependencies(org.apache.ambari.server.api.services.KerberosServiceMetaInfoTest): > Guice provision errors:(..) > > testCommonOozieServiceDescriptor(org.apache.ambari.server.stack.KerberosDescriptorTest): > > /media/plextor/review_ambari/ambari-server/target/classes/common-services/OOZIE/5.0.0.2.3/kerberos.json > is not a readable file > > Tests run: 2951, Failures: 0, Errors: 4, Skipped: 17 > > [INFO] > ------------------------------------------------------------------------ > [INFO] Reactor Summary: > [INFO] > [INFO] Ambari Views ...................................... SUCCESS [2.900s] > [INFO] Ambari Metrics Common ............................. SUCCESS [1.570s] > [INFO] Ambari Server ..................................... FAILURE > [43:59.418s] > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD FAILURE > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 44:04.603s > [INFO] Finished at: Wed May 06 20:25:17 EEST 2015 > [INFO] Final Memory: 32M/268M > [INFO] ---------------------------------------------------------------------- > > > Thanks, > > Dmitro Lisnichenko > >
