You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2020/07/10 04:40:53 UTC
[GitHub] [storm] kishorvpatil opened a new pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
kishorvpatil opened a new pull request #3306:
URL: https://github.com/apache/storm/pull/3306
## What is the purpose of the change
The getClusterInfo method is being invoked where mostly a single call to get _TopologySummary_ by Name or by Id should suffice.
- Added methods to nimbus
- Added impletention with mostly refactoring _getClusternfoImpl_
- Added modified UI and CLI commands
## How was the change tested
- Launched UI and walked through all pages.
- manually tested CLI commands.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [storm] agresch commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r488874655
##########
File path: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java
##########
@@ -40,6 +40,11 @@
"fileUpload",
"getNimbusConf",
"getClusterInfo",
+ "getLeader",
Review comment:
getLeader was added due to a bug below I guess?
##########
File path: storm-core/src/jvm/org/apache/storm/command/GetErrors.java
##########
@@ -42,21 +43,17 @@ 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);
- }
-
- Map<String, Object> outputMap = new HashMap<>();
- if (topologyId == null || topologyInfo == null) {
- outputMap.put("Failure", "No topologies running with name " + name);
- } else {
+ TopologyInfo topologyInfo;
+ Map<String, Object> outputMap = null;
Review comment:
combine these two lines
##########
File path: storm-client/src/py/storm/Nimbus-remote
##########
@@ -75,9 +75,14 @@ if len(sys.argv) <= 1 or sys.argv[1] == '--help':
print(' string downloadChunk(string id)')
print(' string getNimbusConf()')
print(' ClusterSummary getClusterInfo()')
+ print(' getTopologySummaries()')
Review comment:
should these have return types?
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -4747,7 +4866,7 @@ public boolean isTopologyNameAllowed(String name) throws AuthorizationException,
for (StormBase base : ownerToBasesEntry.getValue()) {
try {
String topoId = state.getTopoId(base.get_name())
- .orElseThrow(() -> new WrappedNotAliveException(base.get_name() + " is not alive"));
Review comment:
why change this exception?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r452949990
##########
File path: storm-client/src/jvm/org/apache/storm/ILocalCluster.java
##########
@@ -135,6 +137,12 @@ ILocalTopology submitTopologyWithOpts(String topologyName, Map<String, Object> c
*/
ClusterSummary getClusterInfo() throws TException;
+ java.util.List<TopologySummary> getTopologySummaryInfo() throws AuthorizationException, TException;
Review comment:
suggest to import `java.util.List` and then use List directly
suggest to change to `getTopologySummaries`
can delete `AuthorizationException`
##########
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`?
##########
File path: storm-server/src/main/java/org/apache/storm/LocalCluster.java
##########
@@ -572,11 +573,43 @@ public ClusterSummary getClusterInfo() throws TException {
return getNimbus().getClusterInfo();
}
+ @Override
+ public List<TopologySummary> getTopologySummaries() throws AuthorizationException, TException {
Review comment:
AuthorizationException can be removed
##########
File path: storm-client/src/jvm/org/apache/storm/ILocalCluster.java
##########
@@ -135,6 +137,12 @@ ILocalTopology submitTopologyWithOpts(String topologyName, Map<String, Object> c
*/
ClusterSummary getClusterInfo() throws TException;
+ java.util.List<TopologySummary> getTopologySummaryInfo() throws AuthorizationException, TException;
+
+ TopologySummary getTopologySummaryByName(java.lang.String name) throws AuthorizationException, TException;
+
+ TopologySummary getTopologySummaryById(java.lang.String id) throws AuthorizationException, TException;
Review comment:
`String` should be good enough.
can delete `AuthorizationException` , since it extends `TException`
##########
File path: storm-server/src/main/java/org/apache/storm/LocalCluster.java
##########
@@ -572,11 +573,43 @@ public ClusterSummary getClusterInfo() throws TException {
return getNimbus().getClusterInfo();
}
+ @Override
+ public List<TopologySummary> getTopologySummaries() throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaries();
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryByName(String name) throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaryByName(name);
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryById(String id) throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaryById(id);
+ }
+
@Override
public TopologyInfo getTopologyInfo(String id) throws TException {
return getNimbus().getTopologyInfo(id);
}
+ @Override
+ public TopologyInfo getTopologyInfoByName(String name) throws TException {
+ return getNimbus().getTopologyInfoByName(name);
+ }
+
+ @Override
+ public TopologyInfo getTopologyInfoWithOpts(String id, GetInfoOptions options) throws NotAliveException, AuthorizationException,
+ TException {
+ return nimbus.getTopologyInfoWithOpts(id, options);
+ }
+
+ @Override
+ public TopologyInfo getTopologyInfoByNameWithOpts(String name, GetInfoOptions options) throws NotAliveException, AuthorizationException,
Review comment:
NotAliveException, AuthorizationException can be removed
##########
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 {
Review comment:
looks like we don't need AuthorizationException
##########
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
##########
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 {
Review comment:
looks like we don't need AuthorizationException
##########
File path: storm-client/src/storm.thrift
##########
@@ -777,9 +777,14 @@ service Nimbus {
string getNimbusConf() throws (1: AuthorizationException aze);
// stats functions
ClusterSummary getClusterInfo() throws (1: AuthorizationException aze);
+ list<TopologySummary> getTopologySummaries() throws (1: AuthorizationException aze);
+ TopologySummary getTopologySummaryByName(1: string name) throws (1: AuthorizationException aze);
+ TopologySummary getTopologySummaryById(1: string id) throws (1: AuthorizationException aze);
Review comment:
I think this is better to be `getTopologySummary` (without `ById`) since other functions don't have `ById` when it is getting information by id, e.g. `getTopologyInfo, getTopologyPageInfo, getTopologyConf, getTopology`, etc
##########
File path: examples/storm-starter/src/jvm/org/apache/storm/starter/InOrderDeliveryTest.java
##########
@@ -40,16 +40,11 @@
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) {
+ TopologySummary ts = client.getTopologySummaryByName(name);
+ if (ts == null) {
Review comment:
I beliefve this is no longer needed, since if `getTopologySummaryByName` fails, `getTopologySummaryByName` will throw `TException` or `RuntimeException`.
If this is not the case, then other places where we invoke `getTopologySummaryByName` should also deal with the `result=null`.
##########
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?
##########
File path: storm-client/src/jvm/org/apache/storm/ILocalCluster.java
##########
@@ -135,6 +137,12 @@ ILocalTopology submitTopologyWithOpts(String topologyName, Map<String, Object> c
*/
ClusterSummary getClusterInfo() throws TException;
+ java.util.List<TopologySummary> getTopologySummaryInfo() throws AuthorizationException, TException;
+
+ TopologySummary getTopologySummaryByName(java.lang.String name) throws AuthorizationException, TException;
Review comment:
`String` should be good enough.
can delete `AuthorizationException`, since it extends `TException`
##########
File path: storm-server/src/main/java/org/apache/storm/LocalCluster.java
##########
@@ -572,11 +573,43 @@ public ClusterSummary getClusterInfo() throws TException {
return getNimbus().getClusterInfo();
}
+ @Override
+ public List<TopologySummary> getTopologySummaries() throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaries();
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryByName(String name) throws AuthorizationException, TException {
Review comment:
AuthorizationException can be removed
##########
File path: integration-test/src/test/java/org/apache/storm/st/wrapper/StormCluster.java
##########
@@ -108,7 +108,7 @@ public TopologySummary getOneActive() throws TException {
}
public TopologyInfo getInfo(TopologySummary topologySummary) throws TException {
- return client.getTopologyInfo(topologySummary.get_id());
+ return client.getTopologyInfoByName(topologySummary.get_id());
Review comment:
This doesn't look right.
##########
File path: storm-server/src/main/java/org/apache/storm/LocalCluster.java
##########
@@ -572,11 +573,43 @@ public ClusterSummary getClusterInfo() throws TException {
return getNimbus().getClusterInfo();
}
+ @Override
+ public List<TopologySummary> getTopologySummaries() throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaries();
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryByName(String name) throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaryByName(name);
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryById(String id) throws AuthorizationException, TException {
Review comment:
AuthorizationException can be removed
##########
File path: storm-server/src/main/java/org/apache/storm/LocalCluster.java
##########
@@ -572,11 +573,43 @@ public ClusterSummary getClusterInfo() throws TException {
return getNimbus().getClusterInfo();
}
+ @Override
+ public List<TopologySummary> getTopologySummaries() throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaries();
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryByName(String name) throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaryByName(name);
+ }
+
+ @Override
+ public TopologySummary getTopologySummaryById(String id) throws AuthorizationException, TException {
+ return getNimbus().getTopologySummaryById(id);
+ }
+
@Override
public TopologyInfo getTopologyInfo(String id) throws TException {
return getNimbus().getTopologyInfo(id);
}
+ @Override
+ public TopologyInfo getTopologyInfoByName(String name) throws TException {
+ return getNimbus().getTopologyInfoByName(name);
+ }
+
+ @Override
+ public TopologyInfo getTopologyInfoWithOpts(String id, GetInfoOptions options) throws NotAliveException, AuthorizationException,
Review comment:
NotAliveException, AuthorizationException can be removed
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -4056,6 +4077,45 @@ public TopologyInfo getTopologyInfo(String id) throws NotAliveException, Authori
}
}
+ @Override
+ public TopologyInfo getTopologyInfoByName(String name) throws NotAliveException, AuthorizationException, TException {
+ try {
+ getTopologyInfoByNameCalls.mark();
+ GetInfoOptions options = new GetInfoOptions();
+ options.set_num_err_choice(NumErrorsChoice.ALL);
+
+ return getTopologyInfoByNameImpl(name, options);
+ } catch (Exception e) {
+ LOG.warn("get topology info exception. (topology name={})", name, e);
+ if (e instanceof TException) {
+ throw (TException) e;
+ }
+ throw new RuntimeException(e);
+ }
+ }
+
+ private TopologyInfo getTopologyInfoByNameImpl(String name, GetInfoOptions options) throws
Review comment:
This might should be `getTopologyInfoByNameWithOptsImpl`?
##########
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?
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -502,11 +507,16 @@ public Nimbus(Map<String, Object> conf, INimbus inimbus, IStormClusterState stor
this.getTopologyCalls = metricsRegistry.registerMeter("nimbus:num-getTopology-calls");
this.getUserTopologyCalls = metricsRegistry.registerMeter("nimbus:num-getUserTopology-calls");
this.getClusterInfoCalls = metricsRegistry.registerMeter("nimbus:num-getClusterInfo-calls");
+ this.getTopologySummariesCalls = metricsRegistry.registerMeter("nimbus:num-getTopologySummaries-calls");
Review comment:
We need to update docs/ClusterMetrics.md too
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -4056,6 +4077,45 @@ public TopologyInfo getTopologyInfo(String id) throws NotAliveException, Authori
}
}
+ @Override
+ public TopologyInfo getTopologyInfoByName(String name) throws NotAliveException, AuthorizationException, TException {
+ try {
+ getTopologyInfoByNameCalls.mark();
+ GetInfoOptions options = new GetInfoOptions();
+ options.set_num_err_choice(NumErrorsChoice.ALL);
+
+ return getTopologyInfoByNameImpl(name, options);
+ } catch (Exception e) {
+ LOG.warn("get topology info exception. (topology name={})", name, e);
+ if (e instanceof TException) {
+ throw (TException) e;
+ }
+ throw new RuntimeException(e);
+ }
+ }
+
+ private TopologyInfo getTopologyInfoByNameImpl(String name, GetInfoOptions options) throws
+ NotAliveException, AuthorizationException, TException {
+ IStormClusterState state = stormClusterState;
+ String id = state.getTopoId(name).orElseThrow(() -> new WrappedNotAliveException(name + " is not alive"));
+ return getTopologyInfoWithOpts(id, options);
+ }
+
+ @Override
+ public TopologyInfo getTopologyInfoByNameWithOpts(String name, GetInfoOptions options) throws
+ NotAliveException, AuthorizationException, TException {
Review comment:
looks like we don't need `NotAliveException, AuthorizationException`
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -4056,6 +4077,45 @@ public TopologyInfo getTopologyInfo(String id) throws NotAliveException, Authori
}
}
+ @Override
+ public TopologyInfo getTopologyInfoByName(String name) throws NotAliveException, AuthorizationException, TException {
Review comment:
Looks like we don't need `NotAliveException, AuthorizationException`
##########
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);
+ checkAuthorization(null, null, "getTopology");
Review comment:
Why is this `getTopology` while `getTopologySummaryByName` uses `getTopologySummaries`
Should we have `getTopologySummaryByName` for `getTopologySummaryByName` method and `getTopologySummaryById` for `getTopologySummaryById` method?
##########
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?
##########
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 {
Review comment:
looks like we don't need AuthorizationException
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [storm] kishorvpatil commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
kishorvpatil commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r456077835
##########
File path: integration-test/src/test/java/org/apache/storm/st/wrapper/StormCluster.java
##########
@@ -108,7 +108,7 @@ public TopologySummary getOneActive() throws TException {
}
public TopologyInfo getInfo(TopologySummary topologySummary) throws TException {
- return client.getTopologyInfo(topologySummary.get_id());
+ return client.getTopologyInfoByName(topologySummary.get_id());
Review comment:
fixing.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r467680482
##########
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 `Utils. getTopologyId` should return `null` if the topology is not alive / does not exist.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [storm] Ethanlm merged pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
Ethanlm merged pull request #3306:
URL: https://github.com/apache/storm/pull/3306
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r456659955
##########
File path: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java
##########
@@ -40,6 +40,11 @@
"fileUpload",
"getNimbusConf",
"getClusterInfo",
+ "getLeader",
+ "isTopologyNameAllowed",
+ "getTopologySummaries",
+ "getTopologySummaryByName",
+ "getTopologySummary",
Review comment:
getTopologyInfoByName and getTopologyInfoByNameWithOpts need 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:
users@infra.apache.org
[GitHub] [storm] kishorvpatil commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
kishorvpatil commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r457622150
##########
File path: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java
##########
@@ -40,6 +40,11 @@
"fileUpload",
"getNimbusConf",
"getClusterInfo",
+ "getLeader",
+ "isTopologyNameAllowed",
+ "getTopologySummaries",
+ "getTopologySummaryByName",
+ "getTopologySummary",
Review comment:
All variations for for _getTopologyInfo_ method are using _getTopologyInfo_ for now. I do not intend to change implementation _getTopologyInfoWithOptsImpl_ at this point to refactor checkAuthorization else where.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3306: STORM-3671: Add TopologySummary methods to Nimbus
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3306:
URL: https://github.com/apache/storm/pull/3306#discussion_r467682217
##########
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) if the topology is not alive. So the result will never be null. We need a different check 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:
users@infra.apache.org