falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pallavi Rao" <pallavi....@inmobi.com>
Subject Re: Review Request 38669: Add Utils to common to support native scheduler
Date Thu, 24 Sep 2015 05:30:05 GMT


> On Sept. 23, 2015, 4:04 p.m., Ajay Yadava wrote:
> > common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java, line 414
> > <https://reviews.apache.org/r/38669/diff/1/?file=1082229#file1082229line414>
> >
> >     nit: fully qualified name shouldn't be required I believe.

Yep. Thats the cluster that is in the import. Modified it and a few other methods with similar
usage.


> On Sept. 23, 2015, 4:04 p.m., Ajay Yadava wrote:
> > common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java, line 440
> > <https://reviews.apache.org/r/38669/diff/1/?file=1082229#file1082229line440>
> >
> >     nit: wouldn't separating these two scenarios in two different test cases make
tests more granular and independent of each other?

I added it in the same method as it required the same setup (creation of cluster and process)
as there aren't a whole lot of negative testcases. Didn't want that the setup to repeat (just
adds time to UT).


> On Sept. 23, 2015, 4:04 p.m., Ajay Yadava wrote:
> > common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java, line 431
> > <https://reviews.apache.org/r/38669/diff/1/?file=1082229#file1082229line431>
> >
> >     nit : Now that you have created a constant for the separator shouldn't we be
using that here instead of hardcoding?

Required me to increase the access level of the constant. Addressed it, but, somehow code
looks more verbose :-)


- Pallavi


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


On Sept. 23, 2015, 8:23 a.m., Pallavi Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38669/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2015, 8:23 a.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: FALCON-1483
>     https://issues.apache.org/jira/browse/FALCON-1483
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Utility method that the native scheduler requires.
> 
> This is linked to https://reviews.apache.org/r/35724/. All review comments there have
been incorporated in the patch.
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/falcon/entity/EntityUtil.java ad41674 
>   common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java d022bae 
> 
> Diff: https://reviews.apache.org/r/38669/diff/
> 
> 
> Testing
> -------
> 
> UT added. Manual testing done.
> 
> 
> Thanks,
> 
> Pallavi Rao
> 
>


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