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/04/08 20:59:37 UTC
[GitHub] [storm] Ethanlm opened a new pull request #3247: [STORM-3619] Add
null check for topology name
Ethanlm opened a new pull request #3247: [STORM-3619] Add null check for topology name
URL: https://github.com/apache/storm/pull/3247
https://issues.apache.org/jira/browse/STORM-3619
With this change,
```
Exception in thread "main" java.lang.IllegalArgumentException: Topology name 'null' is not valid. It can't be null and it must match ^[^/.:\\]+$
at org.apache.storm.utils.Utils.validateTopologyName(Utils.java:1821)
at org.apache.storm.StormSubmitter.submitTopologyAs(StormSubmitter.java:225)
at org.apache.storm.StormSubmitter.submitTopology(StormSubmitter.java:210)
at org.apache.storm.StormSubmitter.submitTopologyWithProgressBar(StormSubmitter.java:416)
at org.apache.storm.StormSubmitter.submitTopologyWithProgressBar(StormSubmitter.java:397)
at com.example.SimplifiedTwoCompTopology.main(SimplifiedTwoCompTopology.java:49)
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3247:
[STORM-3619] Add null check for topology name
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3247: [STORM-3619] Add null check for topology name
URL: https://github.com/apache/storm/pull/3247#discussion_r405814652
##########
File path: storm-client/src/jvm/org/apache/storm/utils/Utils.java
##########
@@ -122,7 +122,9 @@
// tests by subclassing.
private static Utils _instance = new Utils();
private static String memoizedLocalHostnameString = null;
- public static final Pattern TOPOLOGY_KEY_PATTERN = Pattern.compile("^[\\w \\t\\._-]+$", Pattern.UNICODE_CHARACTER_CLASS);
+ public static final Pattern BLOB_KEY_PATTERN =
+ Pattern.compile("^[\\w \\t\\._-]+$", Pattern.UNICODE_CHARACTER_CLASS);
+ private static final Pattern TOPOLOGY_NAME_REGEX = Pattern.compile("^[^/.:\\\\]+$");
Review comment:
Copied `TOPOLOGY_NAME_REGEX` from storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java. We should have a consistent check for topology name.
`TOPOLOGY_KEY_PATTERN` was originally only used for blob key. See https://github.com/apache/storm/pull/2703
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3247:
[STORM-3619] Add null check for topology name
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3247: [STORM-3619] Add null check for topology name
URL: https://github.com/apache/storm/pull/3247#discussion_r405812718
##########
File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
##########
@@ -435,7 +436,7 @@ public void onCompleted(String srcFile, String targetFile, long totalBytes) {
});
}
- private static boolean topologyNameExists(String name, NimbusClient client) {
+ private static boolean isTopologyNameAllowed(String name, NimbusClient client) {
Review comment:
Changing the name here because it calls `client.getClient().isTopologyNameAllowed` and it doesn't only check the existence of the topology name.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3247:
[STORM-3619] Add null check for topology name
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3247: [STORM-3619] Add null check for topology name
URL: https://github.com/apache/storm/pull/3247#discussion_r405817261
##########
File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
##########
@@ -248,11 +252,8 @@ public static void submitTopologyAs(String name, Map<String, Object> topoConf, S
try {
String serConf = JSONValue.toJSONString(topoConf);
try (NimbusClient client = NimbusClient.getConfiguredClientAs(conf, asUser)) {
- if (topologyNameExists(name, client)) {
- throw new RuntimeException("Topology with name `" + name + "` already exists on cluster");
- }
- if (!Utils.isValidKey(name)) {
Review comment:
There are two reasons:
1. Moved the name check to the very beginning of this submit function.
2. Replace it with `Utils.validateTopologyName(name);` so we have the same check on client and server side.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm merged pull request #3247: [STORM-3619] Add null
check for topology name
Posted by GitBox <gi...@apache.org>.
Ethanlm merged pull request #3247: [STORM-3619] Add null check for topology name
URL: https://github.com/apache/storm/pull/3247
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [storm] agresch commented on a change in pull request #3247:
[STORM-3619] Add null check for topology name
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3247: [STORM-3619] Add null check for topology name
URL: https://github.com/apache/storm/pull/3247#discussion_r405814951
##########
File path: storm-client/src/jvm/org/apache/storm/StormSubmitter.java
##########
@@ -248,11 +252,8 @@ public static void submitTopologyAs(String name, Map<String, Object> topoConf, S
try {
String serConf = JSONValue.toJSONString(topoConf);
try (NimbusClient client = NimbusClient.getConfiguredClientAs(conf, asUser)) {
- if (topologyNameExists(name, client)) {
- throw new RuntimeException("Topology with name `" + name + "` already exists on cluster");
- }
- if (!Utils.isValidKey(name)) {
Review comment:
why was this removed?
----------------------------------------------------------------
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
With regards,
Apache Git Services