You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by jerrypeng <gi...@git.apache.org> on 2015/10/27 20:13:28 UTC
[GitHub] storm pull request: Validate topology Configs during topology subm...
GitHub user jerrypeng opened a pull request:
https://github.com/apache/storm/pull/825
Validate topology Configs during topology submission
adding config validation during the topology submission process so that incorrect configs can be detected early
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jerrypeng/storm validate_topology_confs_in_submitter
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/825.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #825
----
commit 17df0526aa78112e3883148a40010a01fa5017fa
Author: Boyang Jerry Peng <je...@yahoo-inc.com>
Date: 2015-10-27T19:10:03Z
adding config validation during the topology submission process so that incorrect configs can be detected early
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-154128292
I guess I'm wondering why all those changes are in `ConfigValidation`.
I'm +1, but I'd rather have changes like that in a separate pull request for a separate jira, like 'remove checked exceptions' or something.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-153489817
One minor nit, then I am +1 pending relevant tests pass.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-154128003
+1 looks good to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-154127417
@knusbaum the functionality of ConfigValidation has not changed, but this PR adds support for validating configs submitted with a topology. This allows storm to do early detection on whether the configs submitted with a topology are valid
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-154126854
It doesn't look like `ConfigValidation` has changed functionally at all. Is that correct?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/825#discussion_r43806837
--- Diff: storm-core/src/jvm/backtype/storm/StormSubmitter.java ---
@@ -18,6 +18,7 @@
package backtype.storm;
import java.io.File;
+import java.lang.reflect.InvocationTargetException;
--- End diff --
remove?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/825#discussion_r43772643
--- Diff: storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -558,9 +566,14 @@ public static void validateFields(Map conf) throws IllegalAccessException, Insta
* @param conf map of configs
* @param configClass config class
*/
- public static void validateFields(Map conf, Class configClass) throws IllegalAccessException, InstantiationException, NoSuchFieldException, NoSuchMethodException, InvocationTargetException {
+ public static void validateFields(Map conf, Class configClass) {
for (Field field : configClass.getFields()) {
- Object keyObj = field.get(null);
+ Object keyObj = null;
+ try {
+ keyObj = field.get(null);
+ } catch (IllegalAccessException e) {
+ throw new IllegalStateException("ConfigValidation failed due to: " + e.getMessage() + "\n" + e.getStackTrace().toString());
--- End diff --
Same here, if we just don't want checked exceptions, we can wrap `e` in a RTE.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: Validate topology Configs during topology subm...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-152252726
non related failure in travis for jdk7. Tests for jdk 8 passed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-153488321
@d2r thanks for your review, I just pushed a commit that I think addressed your comments
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/825#discussion_r43772518
--- Diff: storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -504,42 +509,45 @@ public static void validateField(String fieldName, Map conf, Class configClass)
* @param field field that needs to be validated
* @param conf map of confs
*/
- public static void validateField(Field field, Map conf) throws NoSuchFieldException, IllegalAccessException, InstantiationException, NoSuchMethodException, InvocationTargetException {
+ public static void validateField(Field field, Map conf) {
Annotation[] annotations = field.getAnnotations();
if (annotations.length == 0) {
LOG.warn("Field {} does not have validator annotation", field);
}
-
- for (Annotation annotation : annotations) {
- String type = annotation.annotationType().getName();
- Class validatorClass = null;
- Class<?>[] classes = ConfigValidationAnnotations.class.getDeclaredClasses();
- //check if annotation is one of our
- for (Class clazz : classes) {
- if (clazz.getName().equals(type)) {
- validatorClass = clazz;
- break;
- }
- }
- if (validatorClass != null) {
- Object v = validatorClass.cast(annotation);
- String key = (String) field.get(null);
- Class clazz = (Class) validatorClass
- .getMethod(ConfigValidationAnnotations.ValidatorParams.VALIDATOR_CLASS).invoke(v);
- Validator o = null;
- Map<String, Object> params = getParamsFromAnnotation(validatorClass, v);
- //two constructor signatures used to initialize validators.
- //One constructor takes input a Map of arguments, the other doesn't take any arguments (default constructor)
- //If validator has a constructor that takes a Map as an argument call that constructor
- if(hasConstructor(clazz, Map.class)) {
- o = (Validator) clazz.getConstructor(Map.class).newInstance(params);
+ try {
+ for (Annotation annotation : annotations) {
+ String type = annotation.annotationType().getName();
+ Class validatorClass = null;
+ Class<?>[] classes = ConfigValidationAnnotations.class.getDeclaredClasses();
+ //check if annotation is one of our
+ for (Class clazz : classes) {
+ if (clazz.getName().equals(type)) {
+ validatorClass = clazz;
+ break;
+ }
}
- //If not call default constructor
- else {
- o = (((Class<Validator>) clazz).newInstance());
+ if (validatorClass != null) {
+ Object v = validatorClass.cast(annotation);
+ String key = (String) field.get(null);
+ Class clazz = (Class) validatorClass
+ .getMethod(ConfigValidationAnnotations.ValidatorParams.VALIDATOR_CLASS).invoke(v);
+ Validator o = null;
+ Map<String, Object> params = getParamsFromAnnotation(validatorClass, v);
+ //two constructor signatures used to initialize validators.
+ //One constructor takes input a Map of arguments, the other doesn't take any arguments (default constructor)
+ //If validator has a constructor that takes a Map as an argument call that constructor
+ if (hasConstructor(clazz, Map.class)) {
+ o = (Validator) clazz.getConstructor(Map.class).newInstance(params);
+ }
+ //If not call default constructor
+ else {
+ o = (((Class<Validator>) clazz).newInstance());
+ }
+ o.validateField(field.getName(), conf.get(key));
}
- o.validateField(field.getName(), conf.get(key));
}
+ } catch (NoSuchMethodException | IllegalAccessException | InstantiationException | InvocationTargetException e) {
+ throw new IllegalStateException("ConfigValidation failed due to: " + e.getMessage() + "\n" + e.getStackTrace().toString());
--- End diff --
Would this be a operation done in an illegal state? Maybe we could just wrap `e` in a RuntimeException?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-153490830
@d2r thanks again! Just push a commit to fix the one nit
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/825#discussion_r43807074
--- Diff: storm-core/src/jvm/backtype/storm/StormSubmitter.java ---
@@ -18,6 +18,7 @@
package backtype.storm;
import java.io.File;
+import java.lang.reflect.InvocationTargetException;
--- End diff --
will remove
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/825
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-1143] - Validate topology Configs durin...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/825#issuecomment-153511753
Tests passed for java 8. Unrelated test failure for java 7 build
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---