You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Pallavi Rao <pa...@inmobi.com> on 2015/09/23 10:23:50 UTC

Review Request 38669: Add Utils to common to support native scheduler

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

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

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by Pallavi Rao <pa...@inmobi.com>.

> 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
> 
>

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38669/#review100201
-----------------------------------------------------------



common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java (line 414)
<https://reviews.apache.org/r/38669/#comment157358>

    nit: fully qualified name shouldn't be required I believe.



common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java (line 425)
<https://reviews.apache.org/r/38669/#comment157356>

    Should we use getBasePath()?



common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java (line 431)
<https://reviews.apache.org/r/38669/#comment157357>

    nit : Now that you have created a constant for the separator shouldn't we be using that here instead of hardcoding?



common/src/test/java/org/apache/falcon/entity/EntityUtilTest.java (line 440)
<https://reviews.apache.org/r/38669/#comment157359>

    nit: wouldn't separating these two scenarios in two different test cases make tests more granular and independent of each other?


- Ajay Yadava


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
> 
>

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by sandeep samudrala <sa...@gmail.com>.

> On Sept. 23, 2015, 8:34 a.m., sandeep samudrala wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityUtil.java, line 708
> > <https://reviews.apache.org/r/38669/diff/1/?file=1082228#file1082228line708>
> >
> >     ENTITY_MUTATION_TIME_SEPARATOR?
> 
> Pallavi Rao wrote:
>     Wanted to keep it generic.. We could use the same separator if we want to add another part to the dir name.

makes sense


- sandeep


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


On Sept. 24, 2015, 5:31 a.m., Pallavi Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38669/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2015, 5:31 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
> 
>

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by Pallavi Rao <pa...@inmobi.com>.

> On Sept. 23, 2015, 8:34 a.m., sandeep samudrala wrote:
> > common/src/main/java/org/apache/falcon/entity/EntityUtil.java, line 708
> > <https://reviews.apache.org/r/38669/diff/1/?file=1082228#file1082228line708>
> >
> >     ENTITY_MUTATION_TIME_SEPARATOR?

Wanted to keep it generic.. We could use the same separator if we want to add another part to the dir name.


- Pallavi


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


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
> 
>

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by sandeep samudrala <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38669/#review100167
-----------------------------------------------------------



common/src/main/java/org/apache/falcon/entity/EntityUtil.java (line 708)
<https://reviews.apache.org/r/38669/#comment157327>

    ENTITY_MUTATION_TIME_SEPARATOR?


- sandeep samudrala


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
> 
>

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38669/#review100366
-----------------------------------------------------------

Ship it!


Ship It!

- Ajay Yadava


On Sept. 24, 2015, 5:31 a.m., Pallavi Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38669/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2015, 5:31 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
> 
>

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by sandeep samudrala <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38669/#review100369
-----------------------------------------------------------

Ship it!


Ship It!

- sandeep samudrala


On Sept. 24, 2015, 5:31 a.m., Pallavi Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38669/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2015, 5:31 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
> 
>

Re: Review Request 38669: Add Utils to common to support native scheduler

Posted by Pallavi Rao <pa...@inmobi.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38669/
-----------------------------------------------------------

(Updated Sept. 24, 2015, 5:31 a.m.)


Review request for Falcon.


Changes
-------

Review comments addressed.


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 (updated)
-----

  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