Ethanlm commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r462481621
##########
File path: storm-client/src/jvm/org/apache/storm/ILocalCluster.java
##########
@@ -144,6 +154,35 @@ ILocalTopology submitTopologyWithOpts(String topologyName,
Map<String, Object> c
*/
TopologyInfo getTopologyInfo(String id) throws TException;
+ /**
+ * Get the state of a topology.
+ *
+ * @param name the name of the topology (not the id)
+ * @return the state of a topology
+ * @throws TException on any error from nimbus
+ */
+ TopologyInfo getTopologyInfoByName(String name) throws TException;
+
+ /**
+ * Get the state of a topology.
+ *
+ * @param id the id of the topology (not the name)
+ * @param options This is GetInfoOptions to choose Error(s) in on
TopologyInfo.
Review comment:
This comment is a little confusing to me. Does it mean "to choose number
of Errors in TopologyInfo"?
##########
File path: storm-server/src/test/java/org/apache/storm/TestRebalance.java
##########
@@ -43,11 +44,10 @@
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()) {
+ TopologySummary topoSum = cluster.getTopologySummaryByName(topoName);
if (topoSum.get_name().equals(topoName)) {
Review comment:
> Why will the returned topologySummary.get_name != topoName?
I believe this line can be removed
##########
File path: storm-core/src/jvm/org/apache/storm/command/GetErrors.java
##########
@@ -42,15 +42,10 @@ public static void main(String[] args) throws Exception {
public void run(Nimbus.Iface client) throws Exception {
GetInfoOptions opts = new GetInfoOptions();
opts.set_num_err_choice(NumErrorsChoice.ONE);
- String topologyId = Utils.getTopologyId(name, client);
-
- TopologyInfo topologyInfo = null;
- if (topologyId != null) {
- topologyInfo = client.getTopologyInfoWithOpts(topologyId,
opts);
- }
+ TopologyInfo topologyInfo =
client.getTopologyInfoByNameWithOpts(name, opts);
Map<String, Object> outputMap = new HashMap<>();
- if (topologyId == null || topologyInfo == null) {
+ if (topologyInfo == null) {
Review comment:
> Will topologyInfo ever be `null`?
With the implementation in this PR, `getTopologyInfoByNameWithOpts` will
throw an `WrappedNotAliveException` (an TException). So the result will never
be null. We need a different check here.
##########
File path:
examples/storm-starter/src/jvm/org/apache/storm/starter/InOrderDeliveryTest.java
##########
@@ -40,16 +40,8 @@
public class InOrderDeliveryTest {
public static void printMetrics(Nimbus.Iface client, String name) throws
Exception {
- ClusterSummary summary = client.getClusterInfo();
- String id = null;
- for (TopologySummary ts : summary.get_topologies()) {
- if (name.equals(ts.get_name())) {
- id = ts.get_id();
- }
- }
- if (id == null) {
- throw new Exception("Could not find a topology named " + name);
- }
+ TopologySummary ts = client.getTopologySummaryByName(name);
+ String id = ts.get_id();
Review comment:
Can we use ` TopologyInfo info = client.getTopologyInfoByName(name); `
here directly?
instead of
```
TopologySummary ts = client.getTopologySummaryByName(name);
String id = ts.get_id();
TopologyInfo info = client.getTopologyInfo(id);
```
here
##########
File path: storm-client/src/jvm/org/apache/storm/utils/Utils.java
##########
@@ -1188,28 +1188,18 @@ 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();
- }
- }
+ return client.getTopologySummaryByName(name).get_id();
} catch (Exception e) {
throw new RuntimeException(e);
}
- return null;
Review comment:
> I noticed that there is syntax change here. Before the change,
`getTopologyId` returns `null` when it couldn't get the summary. But with the
change, it will throw exceptions when there is no such topology with this
`name`. Is there any impact here?
This change has some impact on backward compatibility since the syntax is
changed here. The caller of this method might reply on the `returned result ==
null` to know if the topology is alive or not.
For example:
https://github.com/apache/storm/blob/master/storm-core/src/jvm/org/apache/storm/command/SetLogLevel.java#L58-L59
User code might do this too. Maybe we should return `null` if the topology
is not alive / does not exist.
##########
File path: storm-server/src/test/java/org/apache/storm/TestRebalance.java
##########
@@ -151,28 +151,22 @@ public void waitTopologyScheduled(String topoName,
ILocalCluster cluster, int re
public boolean checkTopologyScheduled(String topoName, ILocalCluster
cluster) throws TException {
if (checkTopologyUp(topoName, cluster)) {
- ClusterSummary sum = cluster.getClusterInfo();
- for (TopologySummary topoSum : sum.get_topologies()) {
- if (topoSum.get_name().equals(topoName)) {
- String status = topoSum.get_status();
- String sched_status = topoSum.get_sched_status();
- if (status.equals("ACTIVE") && (sched_status != null &&
!sched_status.equals(""))) {
- return true;
- }
- }
+ TopologySummary topoSum =
cluster.getTopologySummaryByName(topoName);
+ String status = topoSum.get_status();
+ String sched_status = topoSum.get_sched_status();
+ if (status.equals("ACTIVE") && (sched_status != null &&
!sched_status.equals(""))) {
+ return true;
}
}
return false;
}
public boolean checkTopologyUp(String topoName, ILocalCluster cluster)
throws TException {
ClusterSummary sum = cluster.getClusterInfo();
-
- for (TopologySummary topoSum : sum.get_topologies()) {
- if (topoSum.get_name().equals(topoName)) {
+ TopologySummary topoSum = cluster.getTopologySummaryByName(topoName);
+ if (topoSum != null) {
Review comment:
> Will it ever be `null`? Looks to me `getTopologySummaryByName` throws
exception if it can't find the topo
Current implementation in the PR will throw exception if the topology can't
be find (when calling `getTopologySummaryByName`). So we need to try-catch
block to determine if this topology is up or not, instead of checking
`topoSum!=null`.
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -2968,16 +2978,14 @@ private SupervisorSummary makeSupervisorSummary(String
supervisorId, SupervisorI
return ret;
}
- private ClusterSummary getClusterInfoImpl() throws Exception {
+ private ClusterSummary getClusterInfoImpl() throws Exception,
AuthorizationException {
Review comment:
> `AuthorizationException` is not needed
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -4654,6 +4714,55 @@ public ClusterSummary getClusterInfo() throws
AuthorizationException, TException
}
}
+ @Override
+ public List<TopologySummary> getTopologySummaries() throws
AuthorizationException, TException {
+ try {
+ getTopologySummariesCalls.mark();
+ checkAuthorization(null, null, "getTopologySummaries");
+ return getTopologySummariesImpl();
+ } catch (Exception e) {
+ LOG.warn("Get TopologySummary info exception.", e);
+ if (e instanceof TException) {
+ throw (TException) e;
+ }
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryByName(String name) throws
AuthorizationException, TException {
+ try {
+ getTopologySummaryByNameCalls.mark();
+ checkAuthorization(null, null, "getTopologySummaries");
+ IStormClusterState state = stormClusterState;
+ String topoId = state.getTopoId(name).orElseThrow(() -> new
WrappedNotAliveException(name + " is not alive"));
+ return getTopologySummary(topoId,
state.topologyBases().get(topoId));
+ } catch (Exception e) {
+ LOG.warn("Get TopologySummaryByName info exception.", e);
+ if (e instanceof TException) {
+ throw (TException) e;
+ }
+ throw new RuntimeException(e);
+ }
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryById(String id) throws
AuthorizationException, TException {
+ try {
+ getTopologySummaryByIdCalls.mark();
+ IStormClusterState state = stormClusterState;
+ StormBase base = state.topologyBases().get(id);
Review comment:
> `base` can be null if this topology doesn't exist. Then there will be
NullPointerException in `getTopologySummary(id, base)` . Is this intended?
We need to check `base==null` here
----------------------------------------------------------------
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:
[email protected]
