[GitHub] [storm] Ethanlm commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus

[GitBox]

Ethanlm commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r489628916



##########
File path: storm-client/src/jvm/org/apache/storm/utils/Utils.java
##########
@@ -1190,23 +1190,17 @@ static boolean isValidConf(Map<String, Object> orig, 
Map<String, Object> deser)
 
     public static TopologyInfo getTopologyInfo(String name, String asUser, 
Map<String, Object> topoConf) {
         try (NimbusClient client = 
NimbusClient.getConfiguredClientAs(topoConf, asUser)) {
-            String topologyId = getTopologyId(name, client.getClient());
-            if (null != topologyId) {
-                return client.getClient().getTopologyInfo(topologyId);
-            }
-            return null;
+            return client.getClient().getTopologyInfoByName(name);
         } catch (Exception e) {
             throw new RuntimeException(e);
         }
     }
 
     public static String getTopologyId(String name, Nimbus.Iface client) {
         try {
-            ClusterSummary summary = client.getClusterInfo();
-            for (TopologySummary s : summary.get_topologies()) {
-                if (s.get_name().equals(name)) {
-                    return s.get_id();
-                }
+            TopologySummary topologySummary = 
client.getTopologySummaryByName(name);

Review comment:
       We should catch NotAliveException and return null if it happens
   This is to keep the syntax consistent with original implementation 

##########
File path: storm-client/src/jvm/org/apache/storm/utils/Utils.java
##########
@@ -1190,23 +1190,17 @@ static boolean isValidConf(Map<String, Object> orig, 
Map<String, Object> deser)
 
     public static TopologyInfo getTopologyInfo(String name, String asUser, 
Map<String, Object> topoConf) {
         try (NimbusClient client = 
NimbusClient.getConfiguredClientAs(topoConf, asUser)) {
-            String topologyId = getTopologyId(name, client.getClient());
-            if (null != topologyId) {
-                return client.getClient().getTopologyInfo(topologyId);
-            }
-            return null;
+            return client.getClient().getTopologyInfoByName(name);

Review comment:
       We should catch NotAliveException and return null if it happens
   This is to keep the syntax consistent with original implementation 

##########
File path: storm-server/src/test/java/org/apache/storm/TestRebalance.java
##########
@@ -43,10 +44,9 @@
     private static final Logger LOG = 
LoggerFactory.getLogger(TestRebalance.class);
 
     public static String topoNameToId(String topoName, ILocalCluster cluster) 
throws TException {
-        for (TopologySummary topoSum : 
cluster.getClusterInfo().get_topologies()) {
-            if (topoSum.get_name().equals(topoName)) {
-                return topoSum.get_id();
-            }
+        TopologySummary topoSum = cluster.getTopologySummaryByName(topoName);

Review comment:
       should we catch NotAliveException and return null if happened?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org