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/19 18:35:08 UTC
[GitHub] storm pull request: [STORM-1111] - Fix Validation for lots of diff...
GitHub user jerrypeng opened a pull request:
https://github.com/apache/storm/pull/807
[STORM-1111] - Fix Validation for lots of different configs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jerrypeng/storm STORM-1111
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/807.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 #807
----
commit 80df44ba5fcfa63dc52bb919c8b0366350e35e58
Author: Boyang Jerry Peng <je...@yahoo-inc.com>
Date: 2015-10-19T16:31:59Z
[STORM-1111] - Fix Validation for lots of different configs
----
---
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-1111] - Fix Validation for lots of diff...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/807#issuecomment-150704659
@HeartSaVioR thanks for your review. I just modified the PR to address 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-1111] - Fix Validation for lots of diff...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/807#discussion_r42669817
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -1215,6 +1254,7 @@
* to be equal to the number of workers configured for this topology. If this variable is set to 0,
* event logging will be disabled.</p>
*/
+ @isPositiveNumber
--- End diff --
This is a really minor nit, but everywhere else `@isInteger` is above `@isPositiveNumber` it would be nice to be consistent.
---
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-1111] - Fix Validation for lots of diff...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/807#discussion_r42770783
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -247,6 +253,7 @@
*
* Defaults to false.
*/
+ @Deprecated
--- End diff --
will remove deprecated annotation
---
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-1111] - Fix Validation for lots of diff...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/807#discussion_r42770893
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -1215,6 +1254,7 @@
* to be equal to the number of workers configured for this topology. If this variable is set to 0,
* event logging will be disabled.</p>
*/
+ @isPositiveNumber
--- End diff --
will fix
---
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-1111] - Fix Validation for lots of diff...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/807#discussion_r42919770
--- Diff: storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -375,7 +401,7 @@ public static void validateField(String name, Class[] keyValidators, Class[] val
}
}
for (Class vv : valueValidators) {
- Object valueValidator = vv.newInstance();
+ Object valueValidator = vv.getConstructor().newInstance();;
--- End diff --
will fix
---
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-1111] - Fix Validation for lots of diff...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/807#discussion_r42917728
--- Diff: storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -375,7 +401,7 @@ public static void validateField(String name, Class[] keyValidators, Class[] val
}
}
for (Class vv : valueValidators) {
- Object valueValidator = vv.newInstance();
+ Object valueValidator = vv.getConstructor().newInstance();;
--- End diff --
nits: ';' appears twice.
---
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-1111] - Fix Validation for lots of diff...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/807
---
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-1111] - Fix Validation for lots of diff...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/807#issuecomment-150001051
Just two minor nits and then I am +1 on this.
---
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-1111] - Fix Validation for lots of diff...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/807#discussion_r42669491
--- Diff: storm-core/src/jvm/backtype/storm/Config.java ---
@@ -247,6 +253,7 @@
*
* Defaults to false.
*/
+ @Deprecated
--- End diff --
This should actually not be deprecated. It says zmq, but it is not ZMQ. It should probably be renamed, but that is a follow on JIRA.
---
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-1111] - Fix Validation for lots of diff...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/807#issuecomment-150280561
@revans2 thanks for your review! I have modified the PR to address 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-1111] - Fix Validation for lots of diff...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/807#issuecomment-150716153
@jerrypeng Nice, +1.
---
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-1111] - Fix Validation for lots of diff...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/807#issuecomment-149309782
unrelated kafka messaging test failed in travis
---
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-1111] - Fix Validation for lots of diff...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/807#discussion_r42917683
--- Diff: storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -425,6 +451,26 @@ public static void validateField(String name, boolean includeZero, Object o) {
}
}
+ public static class MetricRegistryValidator extends Validator {
+
+ @Override
+ public void validateField(String name, Object o) throws IllegalAccessException {
+ if(o == null) {
+ return;
+ }
+ SimpleTypeValidator.validateField(name, Map.class, o);
+ if(!((Map) o).containsKey("class") ) {
+ throw new IllegalAccessException("Field " + name + " must have map entry with key: class");
+ }
+ if(!((Map) o).containsKey("parallelism.hint") ) {
+ throw new IllegalAccessException("Field " + name + " must have map entry with key: class");
--- End diff --
@jerrypeng It should be "key: parallelism.hint".
---
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.
---