Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148442586
@revans2 Thanks! You are right that piece of code involving the try catch
statement is useless. I have removed it
---
If your project is set up for it, you can reply
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42144647
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,570 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42145138
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,570 @@
+/**
+ * Licensed to the Apache Software
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148425413
@d2r @revans2 I just pushed some updates based on what we discussed. Can
you guys look at it again? thanks!
---
If your project is set up for it, you can reply to
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148441476
It looks really good. I am +1 for merging this in. I have one question
about checking that they type is iterable in one place, and then ignoring the
error, to have the
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42148564
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148442889
I am +1 pending not related CI failures.
---
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
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42148487
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42148413
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148462523
The test failure is an issue with maven trying to download something and
getting a failure. I don't see how it is related.
---
If your project is set up for it, you can
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148473108
@revans2 @d2r Thank you for spending the time to review my PR! Much
appreciated!
---
If your project is set up for it, you can reply to this email and have your
reply
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148468577
JDK8 CI test failed because connections to repo.maven.apache.org were
reset, and so dependency resolution did not work. I ran all the tests on OSX
myself and they passed
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/785
---
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
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42014003
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,205 @@
+/**
+ * Licensed to the Apache Software
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148098193
@d2r upmerged and added functionality. Ready for review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42014597
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,205 @@
+/**
+ * Licensed to the Apache
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42027724
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42030910
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42032775
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42032063
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,216 @@
+/**
+ * Licensed to the Apache
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42033497
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42032492
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,216 @@
+/**
+ * Licensed to the Apache
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42042786
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42045972
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148164422
This is nothing you did, but now that the validation is much more readable
I am seeing a few things that should probably be fixed, but I am happy to do
them myself on a
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42042633
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42046068
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42048224
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42045456
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,216 @@
+/**
+ * Licensed to the Apache
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-148180252
Filed https://issues.apache.org/jira/browse/STORM- as a follow on to
have the correct validation on each of the configs.
---
If your project is set up for it, you
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42044717
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,216 @@
+/**
+ * Licensed to the Apache
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r42045623
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,518 @@
+/**
+ * Licensed to the Apache Software Foundation
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-147752280
@knusbaum @d2r @zhuoliu @rfarivar Can review this PR? Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r41886199
--- Diff: storm-core/test/jvm/backtype/storm/TestConfigValidate.java ---
@@ -0,0 +1,137 @@
+package backtype.storm;
+
+import
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r41886669
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,205 @@
+/**
+ * Licensed to the Apache Software
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-147760999
It seems ConfigValidation was changed on master since the merge base for
this branch. Need to check.
---
If your project is set up for it, you can reply to this email and
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r41898760
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidationAnnotations.java
---
@@ -0,0 +1,205 @@
+/**
+ * Licensed to the Apache
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r41898751
--- Diff: storm-core/test/jvm/backtype/storm/TestConfigValidate.java ---
@@ -0,0 +1,137 @@
+package backtype.storm;
+
+import
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-147414223
@revans2 any other suggestions?
---
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
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146926754
@jerrypeng Perhaps we can do a hybrid approach, where we have validation
for complex types made up of something that is a bit ugly, but expressive. And
a set of simple
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146949595
the java annotation spec was written that way
---
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
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146948299
@revans2 I don't think you solution will work either. I don't think we can
get recursion and java annotations to work, since fields in annotations cannot
default to
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146949145
Why can't the default be null?
---
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
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146953738
Crap OK. The JSR has thwarted my plan to take over the universe. What we
have now looks like it is as good as it gets.
---
If your project is set up for it, you can
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146558153
hmmm. You are right, and there is no inheritance in annotations. So we
cannot get that to work. Let me think about it a bit more.
---
If your project is set up for
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r41399786
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,359 @@
+package backtype.storm.validation;
+
+import
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r41397418
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,359 @@
+package backtype.storm.validation;
--- End diff --
Github user jerrypeng commented on a diff in the pull request:
https://github.com/apache/storm/pull/785#discussion_r41399571
--- Diff:
storm-core/src/jvm/backtype/storm/validation/ConfigValidation.java ---
@@ -0,0 +1,359 @@
+package backtype.storm.validation;
--- End diff
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146272172
@revans2 your suggestion are great, but the parameters passed into java
annotations have to declared to be a specific type thus we could not have:
Github user jerrypeng commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146248606
Well for some for annotations I call a common validator and just pass in
the type that needs to be validated, such as @String @Boolean validators.
---
If your
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146249930
Could we use nested annotations for the complex types?
https://blogs.oracle.com/toddfast/entry/creating_nested_complex_java_annotations
```
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146246638
@jerrypeng
I keep going back and forth on how we want the annotations to work. Right
now they are more or less a one to one translation of what we had before.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/785#issuecomment-146250340
@jerrypeng I agree that we want a base set of annotations, and we can
adjust what they do. I am just concerned about making that base set as
flexible and complete as
GitHub user jerrypeng opened a pull request:
https://github.com/apache/storm/pull/785
[STORM-1084] - Improve Storm config validation process to use java
annotations instead of *_SCHEMA format
So currently we specify validators:
public static final String
54 matches
Mail list logo
