> On May 8, 2015, 1:54 p.m., Jonathan Hurley wrote: > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/AlertsDAO.java, > > line 124 > > <https://reviews.apache.org/r/33663/diff/5/?file=953065#file953065line124> > > > > 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.
+1 with the condition that we open a new Jira to track moving away from this pattern and possible using EntityManagerFactory in threads outside of the jetty container. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33663/#review83030 ----------------------------------------------------------- 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 > >
