> 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. > > Robert Levas wrote: > 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.
Reordered in [revision 3](https://reviews.apache.org/r/27700/diff/3/) > 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. > > Robert Levas wrote: > 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. > > John Speidel wrote: > I agree with Bob on this and feel strongly that using sleep statements > like this result in non-deterministic tests. > I haven't really looked at the ActionScheduler to provide you a solution > but can give some genral comments on testing of this nature. > - If you can run the test in a single thread, that is always best. For > example, sometimes it is possible to call run directly. > - If a separate thread must be used, it is best to have a deterministic > way to determine if a task has completed. For example, if you have access to > the task perhaps it has state that can be queried. > - If you must use another thread and have no way to determine with > certainty that a task has completed, use a "wait for" approach. The idea is > that you have a loop that does an assertion every couple of ms. You keep > looping until the assertion holds or until you "timeout" and fail the test. > The timeout should be rather long since it is only applicable in the case of > a test failure. For the successful scenario, the test will not block for long > sine we are checking the condition in small intervals. > > Jonathan Hurley wrote: > Nate and I had a similar issue with testing alert events. Because of the > asynchronous nature of the eventing system, we would have needed to insert > Thread.sleep() in a bunch of tests. Instead, we injected a different event > scheduler which was hard coded to be synchronous. Maybe a similar solution > can be used here where you can inject a single-threaded version of the class > that blocks until its thread completes. Unit tests are single-threaded as of [revision 3](https://reviews.apache.org/r/27700/diff/3/) > 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. > > Robert Levas wrote: > Same response as above.. ;) Unit tests are single-threaded as of [revision 3](https://reviews.apache.org/r/27700/diff/3/) > 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. > > Robert Levas wrote: > Will be reordered. Reordered in [revision 3](https://reviews.apache.org/r/27700/diff/3/) - Robert ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27700/#review60608 ----------------------------------------------------------- On Nov. 11, 2014, 8:43 p.m., Robert Levas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27700/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2014, 8:43 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 > d74510a > > 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 > e1e25e0 > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/MockServerAction.java > PRE-CREATION > > ambari-server/src/test/java/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 > >
