You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by pavan kumar kolamuri <pa...@gmail.com> on 2015/10/14 15:06:03 UTC

Review Request 39316: Delete, Update, Validate entity operations support in Falcon Unit

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

Review request for Falcon.


Bugs: https://issues.apache.org/jira/browse/FALCON-1520
    https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1520


Repository: falcon-git


Description
-------

Added Entity API's in Falcon Unit to support for Integration tests


Diffs
-----

  client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java b889931 
  client/src/main/java/org/apache/falcon/client/FalconCLIException.java ec74c27 
  common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java e27187b 
  oozie/src/main/java/org/apache/oozie/client/LocalProxyOozieClient.java 756828f 
  prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java 3323dd1 
  prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java 0db55df 
  prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java 9d13d74 
  unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 783af19 
  unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java 8b1c435 
  unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java d12efbc 
  unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java d504bd2 
  unit/src/test/resources/process1.xml 37dbb9c 

Diff: https://reviews.apache.org/r/39316/diff/


Testing
-------

Added Unit Tests for these API's


Thanks,

pavan kumar kolamuri


Re: Review Request 39316: Delete, Update, Validate entity operations support in Falcon Unit

Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39316/
-----------------------------------------------------------

(Updated Oct. 26, 2015, 6:18 a.m.)


Review request for Falcon.


Changes
-------

Fixed the testcase failure issue


Bugs: https://issues.apache.org/jira/browse/FALCON-1520
    https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1520


Repository: falcon-git


Description
-------

Added Entity API's in Falcon Unit to support for Integration tests


Diffs (updated)
-----

  client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java b889931 
  client/src/main/java/org/apache/falcon/client/FalconCLIException.java ec74c27 
  common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java e27187b 
  oozie/src/main/java/org/apache/oozie/client/LocalProxyOozieClient.java 756828f 
  prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java 3323dd1 
  prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java 0db55df 
  prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java 9d13d74 
  unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 783af19 
  unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java 8b1c435 
  unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java d12efbc 
  unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java d504bd2 
  unit/src/test/resources/process1.xml 37dbb9c 

Diff: https://reviews.apache.org/r/39316/diff/


Testing
-------

Added Unit Tests for these API's


Thanks,

pavan kumar kolamuri


Re: Review Request 39316: Delete, Update, Validate entity operations support in Falcon Unit

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

Ship it!


Ship It!

- Pallavi Rao


On Oct. 26, 2015, 4:48 a.m., pavan kumar kolamuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39316/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2015, 4:48 a.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-1520
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1520
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Added Entity API's in Falcon Unit to support for Integration tests
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java b889931 
>   client/src/main/java/org/apache/falcon/client/FalconCLIException.java ec74c27 
>   common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java e27187b 
>   oozie/src/main/java/org/apache/oozie/client/LocalProxyOozieClient.java 756828f 
>   prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java 3323dd1 
>   prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java 0db55df 
>   prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java 9d13d74 
>   unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 783af19 
>   unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java 8b1c435 
>   unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java d12efbc 
>   unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java d504bd2 
>   unit/src/test/resources/process1.xml 37dbb9c 
> 
> Diff: https://reviews.apache.org/r/39316/diff/
> 
> 
> Testing
> -------
> 
> Added Unit Tests for these API's
> 
> 
> Thanks,
> 
> pavan kumar kolamuri
> 
>


Re: Review Request 39316: Delete, Update, Validate entity operations support in Falcon Unit

Posted by pavan kumar kolamuri <pa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39316/
-----------------------------------------------------------

(Updated Oct. 26, 2015, 4:48 a.m.)


Review request for Falcon.


Changes
-------

Addressed Comments.


Bugs: https://issues.apache.org/jira/browse/FALCON-1520
    https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1520


Repository: falcon-git


Description
-------

Added Entity API's in Falcon Unit to support for Integration tests


Diffs (updated)
-----

  client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java b889931 
  client/src/main/java/org/apache/falcon/client/FalconCLIException.java ec74c27 
  common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java e27187b 
  oozie/src/main/java/org/apache/oozie/client/LocalProxyOozieClient.java 756828f 
  prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java 3323dd1 
  prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java 0db55df 
  prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java 9d13d74 
  unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 783af19 
  unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java 8b1c435 
  unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java d12efbc 
  unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java d504bd2 
  unit/src/test/resources/process1.xml 37dbb9c 

Diff: https://reviews.apache.org/r/39316/diff/


Testing
-------

Added Unit Tests for these API's


Thanks,

pavan kumar kolamuri


Re: Review Request 39316: Delete, Update, Validate entity operations support in Falcon Unit

Posted by pavan kumar kolamuri <pa...@gmail.com>.

> On Oct. 16, 2015, 9:24 a.m., Pallavi Rao wrote:
> > unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java, line 203
> > <https://reviews.apache.org/r/39316/diff/1/?file=1098225#file1098225line203>
> >
> >     1 instance sufficient?

I want to make sure Coordinator is in running state while deleting. Anyway we are deleting the process in test case , i think 10 would be ok.


> On Oct. 16, 2015, 9:24 a.m., Pallavi Rao wrote:
> > unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java, line 153
> > <https://reviews.apache.org/r/39316/diff/1/?file=1098225#file1098225line153>
> >
> >     Since you are testing scheduling, can you just schedule 1 instance, so the test is faster?

Just making sure Coordinator is in running state . And it's not waiting until 10 instances complete, it will wait only for one instance to complete.


> On Oct. 16, 2015, 9:24 a.m., Pallavi Rao wrote:
> > unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java, line 336
> > <https://reviews.apache.org/r/39316/diff/1/?file=1098224#file1098224line336>
> >
> >     Isn't enum better?

waitForStatus will finally call getStatusOfInstances method which accepts entity type as string . To maintain consistency it has changed to String.


> On Oct. 16, 2015, 9:24 a.m., Pallavi Rao wrote:
> > unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java, line 231
> > <https://reviews.apache.org/r/39316/diff/1/?file=1098222#file1098222line231>
> >
> >     Why has this been changed to String? Isn't it better to use an enum?

This method will call getStatusOfInstances which is in FalconClient. to maintain consistency across all methods it was changed.


> On Oct. 16, 2015, 9:24 a.m., Pallavi Rao wrote:
> > common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java, line 63
> > <https://reviews.apache.org/r/39316/diff/1/?file=1098217#file1098217line63>
> >
> >     Not used by the ConfigurationStore. Better if it's moved to FalconUnitTestBase

Entity Load Order is available in Configuration Store. I thought it will be good to maintain even deletion oredr at same place even though it won't be used as of now.


> On Oct. 16, 2015, 9:24 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java, line 283
> > <https://reviews.apache.org/r/39316/diff/1/?file=1098221#file1098221line283>
> >
> >     BufferedRequest is no longer needed?

Yes, BufferedRequest is not used anywhere in delete method.


> On Oct. 16, 2015, 9:24 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java, line 289
> > <https://reviews.apache.org/r/39316/diff/1/?file=1098221#file1098221line289>
> >
> >     Why is it changed to plain request?

Anyway bufferedRequest won't be used in delete method . Will fix it .


- pavan kumar


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


On Oct. 14, 2015, 1:06 p.m., pavan kumar kolamuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39316/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2015, 1:06 p.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-1520
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1520
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Added Entity API's in Falcon Unit to support for Integration tests
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java b889931 
>   client/src/main/java/org/apache/falcon/client/FalconCLIException.java ec74c27 
>   common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java e27187b 
>   oozie/src/main/java/org/apache/oozie/client/LocalProxyOozieClient.java 756828f 
>   prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java 3323dd1 
>   prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java 0db55df 
>   prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java 9d13d74 
>   unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 783af19 
>   unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java 8b1c435 
>   unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java d12efbc 
>   unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java d504bd2 
>   unit/src/test/resources/process1.xml 37dbb9c 
> 
> Diff: https://reviews.apache.org/r/39316/diff/
> 
> 
> Testing
> -------
> 
> Added Unit Tests for these API's
> 
> 
> Thanks,
> 
> pavan kumar kolamuri
> 
>


Re: Review Request 39316: Delete, Update, Validate entity operations support in Falcon Unit

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



common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java (line 63)
<https://reviews.apache.org/r/39316/#comment160703>

    Not used by the ConfigurationStore. Better if it's moved to FalconUnitTestBase



prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java (line 283)
<https://reviews.apache.org/r/39316/#comment160700>

    BufferedRequest is no longer needed?



prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java (line 289)
<https://reviews.apache.org/r/39316/#comment160701>

    Why is it changed to plain request?



unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java (line 207)
<https://reviews.apache.org/r/39316/#comment160702>

    Why has this been changed to String? Isn't it better to use an enum?



unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java (line 335)
<https://reviews.apache.org/r/39316/#comment160704>

    Isn't enum better?



unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java (line 139)
<https://reviews.apache.org/r/39316/#comment160705>

    Since you are testing scheduling, can you just schedule 1 instance, so the test is faster?



unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java (line 189)
<https://reviews.apache.org/r/39316/#comment160706>

    1 instance sufficient?


- Pallavi Rao


On Oct. 14, 2015, 1:06 p.m., pavan kumar kolamuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39316/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2015, 1:06 p.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-1520
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-1520
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Added Entity API's in Falcon Unit to support for Integration tests
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/falcon/client/AbstractFalconClient.java b889931 
>   client/src/main/java/org/apache/falcon/client/FalconCLIException.java ec74c27 
>   common/src/main/java/org/apache/falcon/entity/store/ConfigurationStore.java e27187b 
>   oozie/src/main/java/org/apache/oozie/client/LocalProxyOozieClient.java 756828f 
>   prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java 3323dd1 
>   prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java 0db55df 
>   prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java 9d13d74 
>   unit/src/main/java/org/apache/falcon/unit/FalconUnitClient.java 783af19 
>   unit/src/main/java/org/apache/falcon/unit/LocalSchedulableEntityManager.java 8b1c435 
>   unit/src/test/java/org/apache/falcon/unit/FalconUnitTestBase.java d12efbc 
>   unit/src/test/java/org/apache/falcon/unit/TestFalconUnit.java d504bd2 
>   unit/src/test/resources/process1.xml 37dbb9c 
> 
> Diff: https://reviews.apache.org/r/39316/diff/
> 
> 
> Testing
> -------
> 
> Added Unit Tests for these API's
> 
> 
> Thanks,
> 
> pavan kumar kolamuri
> 
>