----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38001/#review97576 -----------------------------------------------------------
Ship it! Overall, good job on the patch and I am happy that you have tests for your changes. Couple of comments/questions: - Why not remove cluster name from the table and just have a required cluster ID? I wouldn't expect that we would want to continue to persist the name. - For cases where a BP deployed cluster has already been renamed, it is impossible to recover the cluster ID so this patch only fixes things if it is applied prior to a rename occurring (unless I missed something). As Ambari currently only can manage a single cluster, perhaps we default to the existing cluster if we can't match the name to an ID? - It might be a good idea to remove all usages of cluster name in the topology code since we really only care about the ID. If we really need the name, for a log msg for example, we can get the name using the ID. ambari-server/src/main/java/org/apache/ambari/server/topology/AmbariContext.java (line 390) <https://reviews.apache.org/r/38001/#comment153578> seems that we should only pass in the cluster ID since that is all that we only need the ID and not the topology ambari-server/src/main/java/org/apache/ambari/server/topology/ClusterTopologyImpl.java (line 104) <https://reviews.apache.org/r/38001/#comment153581> I understand why you created this method but I really tried to limit the imports of non-topology related classes here and only access these in AmbariContext. ambari-server/src/main/java/org/apache/ambari/server/topology/TopologyManager.java (line 109) <https://reviews.apache.org/r/38001/#comment153604> I am guessing that this is done here because the topology request is currently persisted prior to the creation of the ambari resources so it wouldn't be possible to get the id from the name when the topology request is persisted. Instead of adding this method, it would be cleaner to move the creation of the ambari resources before the persistance of the topology request so that the id can be obtained from the cluster name in persistedState.persistTopologyRequest if it isn't already set in the request. Then this new method can be removed since the id will be set when the request is persisted. ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog212.java (line 117) <https://reviews.apache.org/r/38001/#comment153586> see above comment about removing the clusterName field - John Speidel On Sept. 1, 2015, 11:53 a.m., Andrew Onischuk wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38001/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2015, 11:53 a.m.) > > > Review request for Ambari, John Speidel and Robert Nettleton. > > > Bugs: AMBARI-12538 > https://issues.apache.org/jira/browse/AMBARI-12538 > > > Repository: ambari > > > Description > ------- > > Attempting to access the Ambari Dashboard results in an HTTP 500 Error after > changing cluster name and restarting the Ambari server > > Refer to <https://issues.apache.org/jira/browse/AMBARI-12538> for more info. > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ExportBlueprintRequest.java > 8b13826 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ProvisionClusterRequest.java > 0150141 > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/ScaleClusterRequest.java > a6995ed > > ambari-server/src/main/java/org/apache/ambari/server/orm/entities/TopologyRequestEntity.java > 8535ed8 > > ambari-server/src/main/java/org/apache/ambari/server/topology/AmbariContext.java > 40717cc > > ambari-server/src/main/java/org/apache/ambari/server/topology/ClusterTopology.java > 9a9929b > > ambari-server/src/main/java/org/apache/ambari/server/topology/ClusterTopologyImpl.java > 9dbd197 > > ambari-server/src/main/java/org/apache/ambari/server/topology/HostRequest.java > cc82a63 > > ambari-server/src/main/java/org/apache/ambari/server/topology/LogicalRequest.java > b7f95cf > > ambari-server/src/main/java/org/apache/ambari/server/topology/PersistedState.java > dbf6735 > > ambari-server/src/main/java/org/apache/ambari/server/topology/PersistedStateImpl.java > 894bc6d > > ambari-server/src/main/java/org/apache/ambari/server/topology/TopologyManager.java > 5f3bb9d > > ambari-server/src/main/java/org/apache/ambari/server/topology/TopologyRequest.java > d102c21 > > ambari-server/src/main/java/org/apache/ambari/server/upgrade/UpgradeCatalog212.java > 02df181 > ambari-server/src/main/resources/Ambari-DDL-MySQL-CREATE.sql 9828eeb > ambari-server/src/main/resources/Ambari-DDL-Oracle-CREATE.sql 22f7493 > ambari-server/src/main/resources/Ambari-DDL-Postgres-CREATE.sql 9fc1239 > ambari-server/src/main/resources/Ambari-DDL-Postgres-EMBEDDED-CREATE.sql > 0c966ca > ambari-server/src/main/resources/Ambari-DDL-SQLAnywhere-CREATE.sql 9f21160 > ambari-server/src/main/resources/Ambari-DDL-SQLServer-CREATE.sql 58760ff > > ambari-server/src/test/java/org/apache/ambari/server/topology/ClusterTopologyImplTest.java > bbea96c > > ambari-server/src/test/java/org/apache/ambari/server/topology/TopologyManagerTest.java > 64dfb28 > > ambari-server/src/test/java/org/apache/ambari/server/upgrade/UpgradeCatalog212Test.java > 7a394ca > > Diff: https://reviews.apache.org/r/38001/diff/ > > > Testing > ------- > > mvn clean test > > > Thanks, > > Andrew Onischuk > >
