----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27700/#review60608 -----------------------------------------------------------
ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionExecutor.java <https://reviews.apache.org/r/27700/#comment101984> I think the methods in this class might need to be re-arranged, such that the public methods are ordered before private methods. I'm not totally sure about this, but most of the Ambari code I've seen follows this standard. ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionExecutor.java <https://reviews.apache.org/r/27700/#comment101976> Should these two timeout values be configurable? Are there any cases where this might require some tuning, based on the size of the cluster being started? ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java <https://reviews.apache.org/r/27700/#comment101990> Is there any way to re-write this test, so that it doesn't spawn a new thread? Since the change is related to concurrency, perhaps there isn't another way around this, but it might make sense to see if this can be tested in a slightly different fashion. I know we've had problems in the past with unit tests that spawn threads, so I thought it made sense to mention this here. ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java <https://reviews.apache.org/r/27700/#comment101991> I'm a little worried by the presence of the sleep here. It might be worth considering making this a functional test instead? Again, just concerned about the unit tests having intermittent failures due to concurrency problems. ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java <https://reviews.apache.org/r/27700/#comment101992> Same concern here about the sleep() call as above. ambari-server/src/test/python/org/apache/ambari/server/serveraction/ServerActionExecutorTest.java <https://reviews.apache.org/r/27700/#comment101994> See my note above regarding the method ordering for classes in Ambari. ambari-server/src/test/python/org/apache/ambari/server/serveraction/ServerActionExecutorTest.java <https://reviews.apache.org/r/27700/#comment101995> Does the mock action object keep these tests from being truly concurrent? Is there any change of an intermittent failure in these types of tests? - Robert Nettleton On Nov. 10, 2014, 2:05 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27700/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2014, 2:05 p.m.) > > > Review request for Ambari, dilli dorai, Jonathan Hurley, John Speidel, Nate > Cole, Robert Nettleton, and Sid Wagle. > > > Bugs: AMBARI-7985 > https://issues.apache.org/jira/browse/AMBARI-7985 > > > Repository: ambari > > > Description > ------- > > Ambari currently handles client-/agent-side commands; however there is no > ability to handle server-side commands. Server-side commands should be > specified as a task in a stage and managed along with the stage. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessor.java > 1f99b4a > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java > 5e879cc > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionManager.java > e2fad5f > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionScheduler.java > 81fee75 > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java > bbc5ac3 > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java > 52b0ba6 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java > 6920a9e > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/AbstractServerAction.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerAction.java > be885b5 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionExecutor.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionManager.java > 011cf06 > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionManagerImpl.java > 3a16c77 > > ambari-server/src/main/java/org/apache/ambari/server/state/ServiceComponentHostEvent.java > 78590fc > > ambari-server/src/main/java/org/apache/ambari/server/state/ServiceComponentHostEventType.java > b43ac9c > > ambari-server/src/main/java/org/apache/ambari/server/state/svccomphost/ServiceComponentHostServerActionEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/svccomphost/ServiceComponentHostUpgradeEvent.java > 8b375fe > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java > 36acbc2 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionManager.java > 5a2c467 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java > 7224924 > > ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatHandler.java > 6e78b1d > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > b2c023f > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/MockServerAction.java > PRE-CREATION > > ambari-server/src/test/python/org/apache/ambari/server/serveraction/ServerActionExecutorTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27700/diff/ > > > Testing > ------- > > ------------------------------------------------------------------------------- > Test set: org.apache.ambari.server.serveraction.ServerActionExecutorTest > ------------------------------------------------------------------------------- > Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 41.822 sec > > > Thanks, > > Robert Levas > >
