[ 
https://issues.apache.org/jira/browse/STORM-966?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14648788#comment-14648788
 ] 

ASF GitHub Bot commented on STORM-966:
--------------------------------------

Github user jerrypeng commented on the pull request:

    https://github.com/apache/storm/pull/658#issuecomment-126573508
  
    Perhaps for the purposes of the config "topology.stats.sample.rate" and 
future number based configs we should have a PositiveNumberValidator.  There is 
already a PositiveIntegerValidator but it only deals with positive integers.  
It seems to me that "topology.stats.sample.rate" can be set to some positive 
float/double or integer. So I think we should include the 
PositiveNumberValidator.  I have added a commit that includes the code for the 
PositiveNumberValidator.  What do you think?


> ConfigValidation.DoubleValidator doesn't really validate whether the type of 
> the object is a double
> ---------------------------------------------------------------------------------------------------
>
>                 Key: STORM-966
>                 URL: https://issues.apache.org/jira/browse/STORM-966
>             Project: Apache Storm
>          Issue Type: Improvement
>            Reporter: Boyang Jerry Peng
>            Assignee: Boyang Jerry Peng
>            Priority: Minor
>
> ConfigValidation.DoubleValidator code only checks if the object is null 
> whether if the object is a instance of Number which is a parent class of 
> Double.
> DoubleValidator is only used once in Config.java and in that instance:
> public static final Object TOPOLOGY_STATS_SAMPLE_RATE_SCHEMA = 
> ConfigValidation.DoubleValidator;
> can just be set to:
> public static final Object TOPOLOGY_STATS_SAMPLE_RATE_SCHEMA = NUMBER.class;
> Then we can just get rid of the misleading function 
> ConfigValidation.DoubleValidator since it doesn't really check if a object is 
> of double type thus the validator function doesn't really do anything and the 
> name is misleading.  In previous commit 
> https://github.com/apache/storm/commit/214ee7454548b884c591991b1faea770d1478cec
>  Number.Class was used anyway



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to