> On April 15, 2015, 12:10 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java, > > lines 73-75 > > <https://reviews.apache.org/r/33205/diff/1/?file=929503#file929503line73> > > > > Why do you need a whole extra ctor that you're just deferring anyway? > > You should also not be passing in the injector, the Factory should handle > > it.
I removed this constructor. > On April 15, 2015, 12:10 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommandFactory.java, > > line 28 > > <https://reviews.apache.org/r/33205/diff/1/?file=929504#file929504line28> > > > > Unnecessary This constructor has several usages because retryAllowed defaults to false. > On April 15, 2015, 12:10 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java, > > line 123 > > <https://reviews.apache.org/r/33205/diff/1/?file=929506#file929506line123> > > > > Not needed. Removed. > On April 15, 2015, 12:10 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/StageFactoryImpl.java, > > lines 32-39 > > <https://reviews.apache.org/r/33205/diff/1/?file=929507#file929507line32> > > > > Not sure why you're doing this. Guice handles the concrete impl of a > > factory and calling the correct constructors for you. You shouldn't be > > passing around the injector and using injector.injectMembers(this) > > everywhere. Thanks for advising me on the correct design pattern. > On April 15, 2015, 12:10 p.m., Nate Cole wrote: > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog210.java, > > lines 142-148 > > <https://reviews.apache.org/r/33205/diff/1/?file=929520#file929520line142> > > > > What is the purpose of this - are you just trying to prove you can get > > to the table? If the host_role_command has any records, then it's possible for there to be a record whose host_name value is "none". In this case, I need to put some host_id, so I pick a random host. On a standalone Ambari setup (only Views), the ambari-upgrade doesn't need this check. - Alejandro ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33205/#review80170 ----------------------------------------------------------- On April 16, 2015, 9:56 p.m., Alejandro Fernandez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33205/ > ----------------------------------------------------------- > > (Updated April 16, 2015, 9:56 p.m.) > > > Review request for Ambari, Andrew Onischuk, Dmitro Lisnichenko, Jonathan > Hurley, Nate Cole, and Sid Wagle. > > > Bugs: AMBARI-10169 > https://issues.apache.org/jira/browse/AMBARI-10169 > > > Repository: ambari > > > Description > ------- > > As part AMBARI-10167 (Delete a host from Ambari cluster is not clean; fails > to re-add the same host), need all of the host-related tables to switch from > a host_name to a host_id. > This is for the host_version and host_role_command tables. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/ActionDBAccessorImpl.java > 7447a2d > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommand.java > f37e937 > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommandFactory.java > 1126666 > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/HostRoleCommandFactoryImpl.java > b63adfa > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/Stage.java > 51d5e8a > > ambari-server/src/main/java/org/apache/ambari/server/actionmanager/StageFactoryImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementController.java > 38c222d > > ambari-server/src/main/java/org/apache/ambari/server/controller/AmbariManagementControllerImpl.java > a4ddf14 > > ambari-server/src/main/java/org/apache/ambari/server/controller/ControllerModule.java > 0c5e04a > > ambari-server/src/main/java/org/apache/ambari/server/controller/KerberosHelper.java > 5cd75bb > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProvider.java > e872fe9 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/UpgradeResourceProvider.java > 9733eff > > ambari-server/src/main/java/org/apache/ambari/server/events/listeners/upgrade/HostVersionOutOfSyncListener.java > dcc06a7 > > ambari-server/src/main/java/org/apache/ambari/server/orm/DBAccessorImpl.java > 279c78f > > ambari-server/src/main/java/org/apache/ambari/server/orm/dao/HostRoleCommandDAO.java > f927197 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostEntity.java > a811c16 > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostRoleCommandEntity.java > c9877fb > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/HostVersionEntity.java > 363e6be > > ambari-server/src/main/java/org/apache/ambari/server/stageplanner/RoleGraph.java > 4fe3787 > > ambari-server/src/main/java/org/apache/ambari/server/stageplanner/RoleGraphFactory.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/stageplanner/RoleGraphFactoryImpl.java > PRE-CREATION > > ambari-server/src/main/java/org/apache/ambari/server/state/cluster/ClusterImpl.java > 1a8bf43 > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog210.java > 884032e > ambari-server/src/main/java/org/apache/ambari/server/utils/StageUtils.java > 020dd4b > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql c3488f2 > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql 0455e9e > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 2c381b2 > ambari-server/src/main/resources/Ambari-DDL-Postgres-EMBEDDED-CREATE.sql > 24762eb > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql adf5828 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/ExecutionCommandWrapperTest.java > d498c97 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/StageTest.java > cd424d4 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionDBAccessorImpl.java > 112e1e5 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionManager.java > 6c5a8a0 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java > dd93176 > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestStage.java > 7ee6045 > > ambari-server/src/test/java/org/apache/ambari/server/agent/AgentResourceTest.java > f4d9c63 > > ambari-server/src/test/java/org/apache/ambari/server/agent/TestHeartbeatHandler.java > c6e2788 > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariCustomCommandExecutionHelperTest.java > 947a76f > > ambari-server/src/test/java/org/apache/ambari/server/controller/AmbariManagementControllerTest.java > 3e310ff > > ambari-server/src/test/java/org/apache/ambari/server/controller/KerberosHelperTest.java > ee11ee7 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/CalculatedStatusTest.java > d11dae0 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/ClusterStackVersionResourceProviderTest.java > 1cc75c7 > ambari-server/src/test/java/org/apache/ambari/server/orm/OrmTestHelper.java > 6041066 > > ambari-server/src/test/java/org/apache/ambari/server/orm/dao/HostVersionDAOTest.java > 7cf59e9 > > ambari-server/src/test/java/org/apache/ambari/server/orm/dao/RequestDAOTest.java > 8ca53f7 > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/ServerActionExecutorTest.java > 580351f > > ambari-server/src/test/java/org/apache/ambari/server/serveraction/upgrades/UpgradeActionTest.java > 87dd18b > > ambari-server/src/test/java/org/apache/ambari/server/stageplanner/TestStagePlanner.java > 0a381f9 > > ambari-server/src/test/java/org/apache/ambari/server/state/cluster/ClusterTest.java > 66a4ade > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeTest.java > 7455706 > > Diff: https://reviews.apache.org/r/33205/diff/ > > > Testing > ------- > > Local unit tests passed on branch that was based on an older commit. > After rebasing, some unit tests failed, so will fix those and re-test on a > live cluster. > > Refactoring the host_version table was easy. > Doing the same for the host_role_command had a cascading effect since many > classes create stages, and that needed the injector to use HostDAO to do the > lookup. > The UpgradeCatalog210 also had changes, and it specifically has one function > that I need to test during the Ambari upgrade. > > > Thanks, > > Alejandro Fernandez > >
