ambari-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Speidel" <jspei...@hortonworks.com>
Subject Re: Review Request 38001: Attempting to access the Ambari Dashboard results in an HTTP 500 Error after changing cluster name and restarting the Ambari server
Date Thu, 03 Sep 2015 00:58:32 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message