> On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionScheduler.java, > > line 190 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579012#file1579012line190> > > > > just "taskStatusLoadedOnce" or "taskStatusLoaded" (since it's boolean > > and can only be false "once")
I am not sure if I fully understand the comment but that variable is used to publish an event to load in progress stages if present in the DB only once during server startup. Considering that comment meant that the name of the variable is confusing, I have changed its name to taskStatusLoaded in the newer revision of the patch. Let me know if I misunderstood the comment. > On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java, > > lines 51-52 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579014#file1579014line51> > > > > Are you using these? no. Thanks for pointing it out Looks my IDE auto remove of unused packages stopped working at some point. I have removed them in the new revision of the patch > On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java, > > lines 303-305 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579014#file1579014line303> > > > > Even though these are simple getters and setters, we should still > > javadoc them Added javadoc for these getters/setters in the new revision of the patch. > On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog300.java, > > lines 110-115 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579025#file1579025line110> > > > > not null, etc Addressed in the new revision of the patch > On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog300.java, > > lines 187-188 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579025#file1579025line187> > > > > no need to break this line up Addressed in the new revision of the patch > On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql, lines 346-347 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579026#file1579026line346> > > > > Should probably make status non-null as well. Addressed in the new revision of the patch > On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/main/resources/Ambari-DDL-Derby-CREATE.sql, lines 363-364 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579026#file1579026line363> > > > > Make not-nullable with a default value of 'PENDING' (a > > HostRoleCommand's initial status is PENDING). Addressed in the new revision of the patch made all host role status for HostRoleCommand, Stage and Request to be non null with default value of PENDING in the schema and entity definition. > On Dec. 13, 2016, 4:31 p.m., Nate Cole wrote: > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java, > > line 58 > > <https://reviews.apache.org/r/53686/diff/3/?file=1579032#file1579032line58> > > > > org.junit.Assert Addressed in the new revision of the patch - Jaimin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53686/#review158848 ----------------------------------------------------------- On Dec. 14, 2016, 2:11 a.m., Jaimin Jetly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53686/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2016, 2:11 a.m.) > > > Review request for Ambari, Jonathan Hurley, Nate Cole, 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 > 2c87583 > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionScheduler.java > e80b020 > > 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 8cf2c0d > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 82ce31e > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql e2c2dd5 > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 4e9a535 > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql 0ba7df6 > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql d8cad6f > > 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 > f86c02e > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeResourceProviderTest.java > e23ba62 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/UpgradeSummaryResourceProviderTest.java > baec7df > > 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 > cc49cbd > > 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 > >
