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 2021/10/28 23:15:07 UTC
[GitHub] [storm] snikhil5 opened a new pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
snikhil5 opened a new pull request #3421:
URL: https://github.com/apache/storm/pull/3421
## What is the purpose of the change
Don't allow deleting blobs if they are required for an active topology
## How was the change tested
Tested on local cluster. Verified that it works as expected. Got the following error message:
"org.apache.storm.utils.WrappedIllegalStateException: Attempting to delete active blob test-delete16acbcb7-91f3-4497-a9bb-67dbfb786948.jar used by topology blob-3-1635454702"
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch merged pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
agresch merged pull request #3421:
URL: https://github.com/apache/storm/pull/3421
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch merged pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
agresch merged pull request #3421:
URL: https://github.com/apache/storm/pull/3421
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741487990
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -3907,6 +3938,12 @@ public void deleteBlob(String key) throws AuthorizationException, KeyNotFoundExc
throw new WrappedIllegalStateException(message);
}
}
+ String topoId = topologyUsingThisBlob(stormClusterState, topoCache, key);
+ if (null != topoId) {
Review comment:
In terms of style null == something or null != something seems dated to when these mistakes (a = null) instead of (a == null) would not get caught by IDE's. But it is harder to read, not necessary anymore, and counter to style elsewhere in the code.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] snikhil5 commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
snikhil5 commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741532149
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -3907,6 +3938,12 @@ public void deleteBlob(String key) throws AuthorizationException, KeyNotFoundExc
throw new WrappedIllegalStateException(message);
}
}
+ String topoId = topologyUsingThisBlob(stormClusterState, topoCache, key);
+ if (null != topoId) {
Review comment:
done
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch merged pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
agresch merged pull request #3421:
URL: https://github.com/apache/storm/pull/3421
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741605090
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -985,6 +985,33 @@ private static boolean isTopologyActiveOrActivating(IStormClusterState state, St
return isTopologyActive(state, topoName) || state.activeStorms().contains(topoName);
}
+ /**
+ * Returns the topologyId of the topology using this blob.
+ * @param state the cluster state
+ * @param topoCache the topology cache
+ * @param key the blob key
+ * @return null or id
+ */
+ private static String topologyUsingThisBlob(IStormClusterState state, TopoCache topoCache, String key) {
+ for (String topologyId : state.activeStorms()) {
+ Map<String, Object> topoConf = null;
+ try {
+ topoConf = readTopoConfAsNimbus(topologyId, topoCache);
+ } catch (KeyNotFoundException | AuthorizationException | IOException e) {
+ continue;
+ }
+ if (topoConf == null) {
+ continue;
+ }
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> blobstoreMap = (Map<String, Map<String, Object>>) topoConf.get(Config.TOPOLOGY_BLOBSTORE_MAP);
+ if (null != blobstoreMap && blobstoreMap.containsKey(key)) {
Review comment:
one last nit: blobstoreMap != null
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] snikhil5 commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
snikhil5 commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741531933
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -985,6 +985,37 @@ private static boolean isTopologyActiveOrActivating(IStormClusterState state, St
return isTopologyActive(state, topoName) || state.activeStorms().contains(topoName);
}
+ /**
+ * Returns the topologyId of the topology using this blob.
+ * @param state the cluster state
+ * @param topoCache the topology cache
+ * @param key the blob key
+ * @return null or id
+ */
+ private static String topologyUsingThisBlob(IStormClusterState state, TopoCache topoCache, String key) {
+ for (String topologyId : state.activeStorms()) {
+ Map<String, Object> topoConf = null;
+ try {
+ topoConf = readTopoConfAsNimbus(topologyId, topoCache);
+ } catch (KeyNotFoundException e) {
+ continue;
+ } catch (AuthorizationException e) {
+ continue;
+ } catch (IOException e) {
+ continue;
+ }
Review comment:
done
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741605090
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -985,6 +985,33 @@ private static boolean isTopologyActiveOrActivating(IStormClusterState state, St
return isTopologyActive(state, topoName) || state.activeStorms().contains(topoName);
}
+ /**
+ * Returns the topologyId of the topology using this blob.
+ * @param state the cluster state
+ * @param topoCache the topology cache
+ * @param key the blob key
+ * @return null or id
+ */
+ private static String topologyUsingThisBlob(IStormClusterState state, TopoCache topoCache, String key) {
+ for (String topologyId : state.activeStorms()) {
+ Map<String, Object> topoConf = null;
+ try {
+ topoConf = readTopoConfAsNimbus(topologyId, topoCache);
+ } catch (KeyNotFoundException | AuthorizationException | IOException e) {
+ continue;
+ }
+ if (topoConf == null) {
+ continue;
+ }
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> blobstoreMap = (Map<String, Map<String, Object>>) topoConf.get(Config.TOPOLOGY_BLOBSTORE_MAP);
+ if (null != blobstoreMap && blobstoreMap.containsKey(key)) {
Review comment:
one last nit: blobstoreMap != null
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -985,6 +985,33 @@ private static boolean isTopologyActiveOrActivating(IStormClusterState state, St
return isTopologyActive(state, topoName) || state.activeStorms().contains(topoName);
}
+ /**
+ * Returns the topologyId of the topology using this blob.
+ * @param state the cluster state
+ * @param topoCache the topology cache
+ * @param key the blob key
+ * @return null or id
+ */
+ private static String topologyUsingThisBlob(IStormClusterState state, TopoCache topoCache, String key) {
+ for (String topologyId : state.activeStorms()) {
+ Map<String, Object> topoConf = null;
+ try {
+ topoConf = readTopoConfAsNimbus(topologyId, topoCache);
+ } catch (KeyNotFoundException | AuthorizationException | IOException e) {
+ continue;
+ }
+ if (topoConf == null) {
+ continue;
+ }
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> blobstoreMap = (Map<String, Map<String, Object>>) topoConf.get(Config.TOPOLOGY_BLOBSTORE_MAP);
+ if (null != blobstoreMap && blobstoreMap.containsKey(key)) {
Review comment:
one last nit: blobstoreMap != null
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741482321
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -985,6 +985,37 @@ private static boolean isTopologyActiveOrActivating(IStormClusterState state, St
return isTopologyActive(state, topoName) || state.activeStorms().contains(topoName);
}
+ /**
+ * Returns the topologyId of the topology using this blob.
+ * @param state the cluster state
+ * @param topoCache the topology cache
+ * @param key the blob key
+ * @return null or id
+ */
+ private static String topologyUsingThisBlob(IStormClusterState state, TopoCache topoCache, String key) {
+ for (String topologyId : state.activeStorms()) {
+ Map<String, Object> topoConf = null;
+ try {
+ topoConf = readTopoConfAsNimbus(topologyId, topoCache);
+ } catch (KeyNotFoundException e) {
+ continue;
+ } catch (AuthorizationException e) {
+ continue;
+ } catch (IOException e) {
+ continue;
+ }
Review comment:
I think it will be more compact to catch all on same line:
catch (KeyNotFoundException | AuthorizationException | IOException e) {
continue;
}
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] snikhil5 commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
snikhil5 commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741531933
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -985,6 +985,37 @@ private static boolean isTopologyActiveOrActivating(IStormClusterState state, St
return isTopologyActive(state, topoName) || state.activeStorms().contains(topoName);
}
+ /**
+ * Returns the topologyId of the topology using this blob.
+ * @param state the cluster state
+ * @param topoCache the topology cache
+ * @param key the blob key
+ * @return null or id
+ */
+ private static String topologyUsingThisBlob(IStormClusterState state, TopoCache topoCache, String key) {
+ for (String topologyId : state.activeStorms()) {
+ Map<String, Object> topoConf = null;
+ try {
+ topoConf = readTopoConfAsNimbus(topologyId, topoCache);
+ } catch (KeyNotFoundException e) {
+ continue;
+ } catch (AuthorizationException e) {
+ continue;
+ } catch (IOException e) {
+ continue;
+ }
Review comment:
done
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -3907,6 +3938,12 @@ public void deleteBlob(String key) throws AuthorizationException, KeyNotFoundExc
throw new WrappedIllegalStateException(message);
}
}
+ String topoId = topologyUsingThisBlob(stormClusterState, topoCache, key);
+ if (null != topoId) {
Review comment:
done
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3421: STORM-3804 Don't allow deleting blobs if they are required for an active topology
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3421:
URL: https://github.com/apache/storm/pull/3421#discussion_r741605090
##########
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##########
@@ -985,6 +985,33 @@ private static boolean isTopologyActiveOrActivating(IStormClusterState state, St
return isTopologyActive(state, topoName) || state.activeStorms().contains(topoName);
}
+ /**
+ * Returns the topologyId of the topology using this blob.
+ * @param state the cluster state
+ * @param topoCache the topology cache
+ * @param key the blob key
+ * @return null or id
+ */
+ private static String topologyUsingThisBlob(IStormClusterState state, TopoCache topoCache, String key) {
+ for (String topologyId : state.activeStorms()) {
+ Map<String, Object> topoConf = null;
+ try {
+ topoConf = readTopoConfAsNimbus(topologyId, topoCache);
+ } catch (KeyNotFoundException | AuthorizationException | IOException e) {
+ continue;
+ }
+ if (topoConf == null) {
+ continue;
+ }
+ @SuppressWarnings("unchecked")
+ Map<String, Map<String, Object>> blobstoreMap = (Map<String, Map<String, Object>>) topoConf.get(Config.TOPOLOGY_BLOBSTORE_MAP);
+ if (null != blobstoreMap && blobstoreMap.containsKey(key)) {
Review comment:
one last nit: blobstoreMap != null
--
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.
To unsubscribe, e-mail: dev-unsubscribe@storm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org