You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Ajay Yadava <aj...@gmail.com> on 2014/11/06 05:08:17 UTC

Re: Review Request 26912: FALCON-722: Add SLA for process

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

(Updated Nov. 6, 2014, 4:08 a.m.)


Review request for Falcon and Srikanth Sundarrajan.


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


Repository: falcon-git


Description (updated)
-------

Add SLA for process. 


Diffs (updated)
-----

  client/src/main/resources/process-0.1.xsd 06a2fe4 
  common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 9be4e85 
  common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java 80a9cc7 
  common/src/test/resources/config/process/process-0.1.xml 99a0376 
  docs/src/site/twiki/EntitySpecification.twiki b0dfb7f 

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


Testing
-------

Yes. All tests pass.


Thanks,

Ajay Yadava

Re: Review Request 26912: FALCON-722: Add SLA for process

Posted by Ajay Yadava <aj...@gmail.com>.

> On Nov. 9, 2014, 3:06 p.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java, line 117
> > <https://reviews.apache.org/r/26912/diff/3/?file=751521#file751521line117>
> >
> >     Variable names are still slaStart/slaEnd.
> >     
> >     Looks like slaStart is validated only if slaEnd is populated. Are we proposing that slaStart has no significance unless slaEnd is not specified
> 
> Ajay Yadava wrote:
>     These are only inner temporary variables. All the error messages and XML attributes have shouldStartIn and shouldEndIn. For inner temporary variables I tried to keep them small instead of shouldStartInExpression or shouldEndInExpression. If it looks confusing or inconsistent then please let me know and I will happily change it.
>     
>     The validation checks that slaStart is less than slaEnd and doesn't make sense if slaEnd is not there.

I just now realized that after refactoring the validations are no longer comprehensive. I will change the variable names and fix the validations as well. Will upload a new patch ASAP


- Ajay


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


On Nov. 6, 2014, 9:31 a.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26912/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2014, 9:31 a.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-722
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-722
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Add SLA for process. 
> 
> 
> Diffs
> -----
> 
>   client/src/main/resources/process-0.1.xsd 06a2fe4 
>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 9be4e85 
>   common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java 80a9cc7 
>   common/src/test/resources/config/process/process-0.1.xml 99a0376 
>   docs/src/site/twiki/EntitySpecification.twiki b0dfb7f 
> 
> Diff: https://reviews.apache.org/r/26912/diff/
> 
> 
> Testing
> -------
> 
> Yes. All tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>

Re: Review Request 26912: FALCON-722: Add SLA for process

Posted by Srikanth Sundarrajan <sr...@hotmail.com>.

> On Nov. 9, 2014, 3:06 p.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java, line 117
> > <https://reviews.apache.org/r/26912/diff/3/?file=751521#file751521line117>
> >
> >     Variable names are still slaStart/slaEnd.
> >     
> >     Looks like slaStart is validated only if slaEnd is populated. Are we proposing that slaStart has no significance unless slaEnd is not specified
> 
> Ajay Yadava wrote:
>     These are only inner temporary variables. All the error messages and XML attributes have shouldStartIn and shouldEndIn. For inner temporary variables I tried to keep them small instead of shouldStartInExpression or shouldEndInExpression. If it looks confusing or inconsistent then please let me know and I will happily change it.
>     
>     The validation checks that slaStart is less than slaEnd and doesn't make sense if slaEnd is not there.
> 
> Ajay Yadava wrote:
>     I just now realized that after refactoring the validations are no longer comprehensive. I will change the variable names and fix the validations as well. Will upload a new patch ASAP

>> The validation checks that slaStart is less than slaEnd and doesn't make sense if slaEnd is not there.

Agreed


- Srikanth


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


On Nov. 9, 2014, 6:36 p.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26912/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2014, 6:36 p.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-722
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-722
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Add SLA for process. 
> 
> 
> Diffs
> -----
> 
>   client/src/main/resources/process-0.1.xsd 06a2fe4 
>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 55887ac 
>   common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java e3a2cd5 
>   common/src/test/resources/config/process/process-0.1.xml 99a0376 
>   docs/src/site/twiki/EntitySpecification.twiki a81a626 
> 
> Diff: https://reviews.apache.org/r/26912/diff/
> 
> 
> Testing
> -------
> 
> Yes. All tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>

Re: Review Request 26912: FALCON-722: Add SLA for process

Posted by Ajay Yadava <aj...@gmail.com>.

> On Nov. 9, 2014, 3:06 p.m., Srikanth Sundarrajan wrote:
> > common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java, line 117
> > <https://reviews.apache.org/r/26912/diff/3/?file=751521#file751521line117>
> >
> >     Variable names are still slaStart/slaEnd.
> >     
> >     Looks like slaStart is validated only if slaEnd is populated. Are we proposing that slaStart has no significance unless slaEnd is not specified

These are only inner temporary variables. All the error messages and XML attributes have shouldStartIn and shouldEndIn. For inner temporary variables I tried to keep them small instead of shouldStartInExpression or shouldEndInExpression. If it looks confusing or inconsistent then please let me know and I will happily change it.

The validation checks that slaStart is less than slaEnd and doesn't make sense if slaEnd is not there.


- Ajay


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


On Nov. 6, 2014, 9:31 a.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26912/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2014, 9:31 a.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-722
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-722
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Add SLA for process. 
> 
> 
> Diffs
> -----
> 
>   client/src/main/resources/process-0.1.xsd 06a2fe4 
>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 9be4e85 
>   common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java 80a9cc7 
>   common/src/test/resources/config/process/process-0.1.xml 99a0376 
>   docs/src/site/twiki/EntitySpecification.twiki b0dfb7f 
> 
> Diff: https://reviews.apache.org/r/26912/diff/
> 
> 
> Testing
> -------
> 
> Yes. All tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>

Re: Review Request 26912: FALCON-722: Add SLA for process

Posted by Srikanth Sundarrajan <sr...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26912/#review60508
-----------------------------------------------------------



common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java
<https://reviews.apache.org/r/26912/#comment101899>

    Variable names are still slaStart/slaEnd.
    
    Looks like slaStart is validated only if slaEnd is populated. Are we proposing that slaStart has no significance unless slaEnd is not specified


- Srikanth Sundarrajan


On Nov. 6, 2014, 9:31 a.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26912/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2014, 9:31 a.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-722
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-722
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Add SLA for process. 
> 
> 
> Diffs
> -----
> 
>   client/src/main/resources/process-0.1.xsd 06a2fe4 
>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 9be4e85 
>   common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java 80a9cc7 
>   common/src/test/resources/config/process/process-0.1.xml 99a0376 
>   docs/src/site/twiki/EntitySpecification.twiki b0dfb7f 
> 
> Diff: https://reviews.apache.org/r/26912/diff/
> 
> 
> Testing
> -------
> 
> Yes. All tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>

Re: Review Request 26912: FALCON-722: Add SLA for process

Posted by Srikanth Sundarrajan <sr...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26912/#review60532
-----------------------------------------------------------



common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java
<https://reviews.apache.org/r/26912/#comment101907>

    This is nested inside shouldStartExpression != null. So does that mean shouldEnd have relevance unless shouldStart is filled in ? Shouldn't this condition (shouldEnd != null) and associated checks be only based on the condition that sla is configured.


- Srikanth Sundarrajan


On Nov. 9, 2014, 6:36 p.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26912/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2014, 6:36 p.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-722
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-722
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Add SLA for process. 
> 
> 
> Diffs
> -----
> 
>   client/src/main/resources/process-0.1.xsd 06a2fe4 
>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 55887ac 
>   common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java e3a2cd5 
>   common/src/test/resources/config/process/process-0.1.xml 99a0376 
>   docs/src/site/twiki/EntitySpecification.twiki a81a626 
> 
> Diff: https://reviews.apache.org/r/26912/diff/
> 
> 
> Testing
> -------
> 
> Yes. All tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>

Re: Review Request 26912: FALCON-722: Add SLA for process

Posted by Srikanth Sundarrajan <sr...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26912/#review60535
-----------------------------------------------------------

Ship it!


Ship It!

- Srikanth Sundarrajan


On Nov. 9, 2014, 6:36 p.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26912/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2014, 6:36 p.m.)
> 
> 
> Review request for Falcon and Srikanth Sundarrajan.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/FALCON-722
>     https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/FALCON-722
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> Add SLA for process. 
> 
> 
> Diffs
> -----
> 
>   client/src/main/resources/process-0.1.xsd 06a2fe4 
>   common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 55887ac 
>   common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java e3a2cd5 
>   common/src/test/resources/config/process/process-0.1.xml 99a0376 
>   docs/src/site/twiki/EntitySpecification.twiki a81a626 
> 
> Diff: https://reviews.apache.org/r/26912/diff/
> 
> 
> Testing
> -------
> 
> Yes. All tests pass.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>

Re: Review Request 26912: FALCON-722: Add SLA for process

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

(Updated Nov. 9, 2014, 6:36 p.m.)


Review request for Falcon and Srikanth Sundarrajan.


Changes
-------

rebased the patch wrt to master. changed variable names. Altered the validation sequence to check for timeout even when shouldEndIn is not specified.


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


Repository: falcon-git


Description
-------

Add SLA for process. 


Diffs (updated)
-----

  client/src/main/resources/process-0.1.xsd 06a2fe4 
  common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 55887ac 
  common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java e3a2cd5 
  common/src/test/resources/config/process/process-0.1.xml 99a0376 
  docs/src/site/twiki/EntitySpecification.twiki a81a626 

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


Testing
-------

Yes. All tests pass.


Thanks,

Ajay Yadava

Re: Review Request 26912: FALCON-722: Add SLA for process

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

(Updated Nov. 6, 2014, 9:31 a.m.)


Review request for Falcon and Srikanth Sundarrajan.


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


Repository: falcon-git


Description
-------

Add SLA for process. 


Diffs (updated)
-----

  client/src/main/resources/process-0.1.xsd 06a2fe4 
  common/src/main/java/org/apache/falcon/entity/parser/ProcessEntityParser.java 9be4e85 
  common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java 80a9cc7 
  common/src/test/resources/config/process/process-0.1.xml 99a0376 
  docs/src/site/twiki/EntitySpecification.twiki b0dfb7f 

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


Testing
-------

Yes. All tests pass.


Thanks,

Ajay Yadava