> On Nov. 10, 2014, 11:32 a.m., Robert Nettleton wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionExecutor.java, > > line 41 > > <https://reviews.apache.org/r/27700/diff/2/?file=754419#file754419line41> > > > > 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.
I didn't notice this convention in the coding guide Wiki - https://cwiki.apache.org/confluence/display/AMBARI/Coding+Guidelines+for+Ambari - but will reorder my methods. > On Nov. 10, 2014, 11:32 a.m., Robert Nettleton wrote: > > ambari-server/src/main/java/org/apache/ambari/server/serveraction/ServerActionExecutor.java, > > line 44 > > <https://reviews.apache.org/r/27700/diff/2/?file=754419#file754419line44> > > > > 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? The time out values are configurable: * EXECUTION_TIMEOUT_MS - from determineTimeout method, where the EXECUTION_TIMEOUT_MS is used if a timeout value is not set in the command parameters (ExecutionCommand.KeyNames.COMMAND_TIMEOUT) * POLLING_TIMEOUT_MS - from the constructor. > On Nov. 10, 2014, 11:32 a.m., Robert Nettleton wrote: > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java, > > line 305 > > <https://reviews.apache.org/r/27700/diff/2/?file=754426#file754426line305> > > > > 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. Similar tests to this create a thread as well - see testHostRoleScheduled and testCustomActionScheduled. > On Nov. 10, 2014, 11:32 a.m., Robert Nettleton wrote: > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java, > > line 654 > > <https://reviews.apache.org/r/27700/diff/2/?file=754428#file754428line654> > > > > 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. We really need to sleep here so that the scheduler and executer have time to run - which are in different threads. The idea is that a tasks a scheuled and we are polling its status to see when it is completed.. then we make sure its status is what was expected. I don't think there is another way around it. > On Nov. 10, 2014, 11:32 a.m., Robert Nettleton wrote: > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java, > > line 740 > > <https://reviews.apache.org/r/27700/diff/2/?file=754428#file754428line740> > > > > Same concern here about the sleep() call as above. Same response as above.. ;) > On Nov. 10, 2014, 11:32 a.m., Robert Nettleton wrote: > > ambari-server/src/test/python/org/apache/ambari/server/serveraction/ServerActionExecutorTest.java, > > line 47 > > <https://reviews.apache.org/r/27700/diff/2/?file=754432#file754432line47> > > > > See my note above regarding the method ordering for classes in Ambari. Will be reordered. > On Nov. 10, 2014, 11:32 a.m., Robert Nettleton wrote: > > ambari-server/src/test/python/org/apache/ambari/server/serveraction/ServerActionExecutorTest.java, > > line 159 > > <https://reviews.apache.org/r/27700/diff/2/?file=754432#file754432line159> > > > > 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? The MockServerAction does not care about being in a thread or not. Technically I might even consider removing the MockServerAction class and using a Mock instead. - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27700/#review60608 ----------------------------------------------------------- On Nov. 10, 2014, 9:05 a.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27700/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2014, 9:05 a.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 > >
