----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53686/#review164025 -----------------------------------------------------------
Have you tried this patch on a large cluster with success? ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java (line 520) <https://reviews.apache.org/r/53686/#comment235539> This shouldn't be a static helper call, it should just be on the status itself: "existingTaskStatus.isValidTransition()" or rather just "!existingTaskStatus.isCompletedState()" ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleStatus.java (line 186) <https://reviews.apache.org/r/53686/#comment235540> See other comment, either make this non-static on the status enum or just use !isCompletedState() directly. ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CalculatedStatus.java (line 310) <https://reviews.apache.org/r/53686/#comment235541> nit: Can remove redundant types here, and a lot of other places in this patch: Map<StatusType, Map<HostRoleStatus, Integer>> counters = new HashMap<>(); List<String> list = new ArrayList<> etc etc ambari-server/src/main/java/org/apache/ambari/server/events/listeners/tasks/TaskStatusListener.java (lines 215 - 216) <https://reviews.apache.org/r/53686/#comment235545> Potentially doing db work to multiple stages. What are the implications of failure here? Should it be @Transactional (if yes, remember the method cannot be public). Also, if some of them can be updated, and others not, can they be reconciled to "try again"? ambari-server/src/main/java/org/apache/ambari/server/events/listeners/tasks/TaskStatusListener.java (lines 260 - 262) <https://reviews.apache.org/r/53686/#comment235547> Same as other comment. Transactional? Partial updates? ambari-server/src/main/java/org/apache/ambari/server/events/publishers/TaskEventPublisher.java (lines 35 - 38) <https://reviews.apache.org/r/53686/#comment235544> This means that task events will happen on the same thread they came in on. Is that ok? We can still make this an async bus with one thread so it won't hold up processing of large amounts of Stages and HostRoleCommands. ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java (lines 647 - 650) <https://reviews.apache.org/r/53686/#comment235553> Since you're deferring to mergeWithoutPublishEvent() that is marked @Transactional already, this method may not need those annotations. @Jonathan for input if it's required for both methods. - Nate Cole On Jan. 18, 2017, 5:33 p.m., Jaimin Jetly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53686/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2017, 5:33 p.m.) > > > Review request for Ambari, Jonathan Hurley, Nate Cole, Sumit Mohanty, and Sid > Wagle. > > > Bugs: AMBARI-18868 > https://issues.apache.org/jira/browse/AMBARI-18868 > > > Repository: ambari > > > Description > ------- > > Stage and Request status should be persisted in the database. > > upgrading to ambari-3.0.0 should add status for all present stages and > request for the cluster. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java > 7837a7b > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionScheduler.java > dabcb98 > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleStatus.java > 3656bfe > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Request.java > 31e11c1 > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java > 4a05b32 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/CalculatedStatus.java > 3c415df > > ambari-server/src/main/java/org/apache/ambari/server/events/TaskCreateEvent.java > PRE-CREATION > ambari-server/src/main/java/org/apache/ambari/server/events/TaskEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/events/TaskUpdateEvent.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/tasks/TaskStatusListener.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/events/publishers/TaskEventPublisher.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java > 02c4091 > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/RequestDAO.java > 1c4d0a3 > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/StageDAO.java > d2f899f > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java > 74271b9 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/RequestEntity.java > 7944d21 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/StageEntity.java > f9c8810 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/StageEntityPK.java > 9ca0470 > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog300.java > 4f90ef3 > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql b79c945 > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 1c502bc > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql c6d4ad0 > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 1be87bb > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql abe48e8 > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql 169a464 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java > 1ca777d > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java > 6cc511e > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java > a702e6f > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeSummaryResourceProviderTest.java > e398a54 > > ambari-server/src/test/java/org/apache/ambari/server/events/listeners/tasks/TaskStatusListenerTest.java > PRE-CREATION > > ambari-server/src/test/java/org/apache/ambari/server/orm/dao/UpgradeDAOTest.java > ae85241 > > ambari-server/src/test/java/org/apache/ambari/server/state/services/RetryUpgradeActionServiceTest.java > 7dd9932 > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog300Test.java > d7979e8 > > Diff: https://reviews.apache.org/r/53686/diff/ > > > Testing > ------- > > Verified manually on a cluster by making api requests and upgrading ambari. > Add unit tests. > Verified that the patch does not break any existing unit tests on dev box. > Jenkins job overall unit test result pending.. > Verified on 1000 node cluster that the patch does not regress big operations. > Executed Stop Services and Start Services API call which gernerated around > 9000 tasks and compared request completion time for these operations. There > was a minor performance gain with the patch. As part of > https://issues.apache.org/jira/browse/AMBARI-18889, I will look if we can use > the request status and stage status to further enhance performance. > > > Thanks, > > Jaimin Jetly > >
