falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Venkat Ranganathan" <n....@live.com>
Subject Re: Review Request 38391: Simple validation for cluster entity properties
Date Wed, 16 Sep 2015 05:16:21 GMT


> On Sept. 15, 2015, 8:59 p.m., Venkat Ranganathan wrote:
> > common/src/main/java/org/apache/falcon/entity/parser/ClusterEntityParser.java, line
266
> > <https://reviews.apache.org/r/38391/diff/2/?file=1073948#file1073948line266>
> >
> >     Any specific reason this is now protected?   Is there an expectation to use
it in derived classes in future?
> 
> Balu Vellanki wrote:
>     It is now protected so that it can be accessed from unit tests.

That is good, but if these methods were private for a reason do we need to dilute the access.
  That is, would a public/protected method that encampasses the access to the private methods
be the method to target in our tests and have the results of that method reflect the variations
we want to test.   It is more of a philosphical question as I see too many of the dilution
of access for purposes of mock testing in a few projects.

A thought for future patches :)


- Venkat


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38391/#review99162
-----------------------------------------------------------


On Sept. 15, 2015, 9:04 a.m., Balu Vellanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38391/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2015, 9:04 a.m.)
> 
> 
> Review request for Falcon, Pallavi Rao, Sowmya Ramesh, and Venkat Ranganathan.
> 
> 
> Bugs: FALCON-1342
>     https://issues.apache.org/jira/browse/FALCON-1342
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> When specifying properties for a cluster, this is currently allowed,
> 
> {code}
> <properties>
>         <property name="test" value="value1"/>
>         <property name="test" value="value2"/>
> </properties>
> {code}
> 
> The propeties are stored as an array of org.apache.falcon.entity.v0.cluster.Property,
and cluster.getProperty("test") will return either "value1" or "value2" but not both. If falcon
does not support multiple values for same property key,  parsing such an entity should throw
an error.
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/falcon/entity/parser/ClusterEntityParser.java 5756f84

>   common/src/main/java/org/apache/falcon/entity/parser/FeedEntityParser.java f22a343

>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 56cc4ca

>   common/src/test/java/org/apache/falcon/entity/parser/ClusterEntityParserTest.java 638cef9

>   common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java 754fb06

>   common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java 6612b74

>   common/src/test/resources/config/feed/feed-0.1.xml 6448803 
> 
> Diff: https://reviews.apache.org/r/38391/diff/
> 
> 
> Testing
> -------
> 
> Added unit test. Tested end2end
> 
> 
> Thanks,
> 
> Balu Vellanki
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message