You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@oozie.apache.org by Ryota Egashira <eg...@yahoo-inc.com> on 2013/05/17 22:28:57 UTC

Re: Review Request: OOZIE-1296 SLA JMS Event Listener for publishing notifications related to SLA information

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

(Updated May 17, 2013, 8:28 p.m.)


Review request for oozie.


Changes
-------

rebased patch at this point.
will upload final version once OOZIE-1244 goes in.
remaining to-do
- change SLAEventImpl in test case to SLARegistrationBean
- modify init() in JMSSLAEventListener


Description
-------

https://issues.apache.org/jira/browse/OOZIE-1296

note that this patch depends on OOZIE-1209, OOZIE-1234, which are not committed yet.
doing modification on new classes created by OOZIE-1209, OOZIE-1234, (Event, SLAEvent,MessageFactory, JMSMessagingUtils, MessageDeserializer, etc), but since these classes not committed on trunk, RB doesn't take patch for only my diff part, thus, including entire class. please be aware.

still WIP, need to work on test case.


This addresses bug OOZIE-1296.
    https://issues.apache.org/jira/browse/OOZIE-1296


Diffs (updated)
-----

  trunk/client/src/main/java/org/apache/oozie/client/event/jms/JMSHeaderConstants.java 1483676 
  trunk/client/src/main/java/org/apache/oozie/client/event/jms/JSONMessageDeserializer.java 1483676 
  trunk/client/src/main/java/org/apache/oozie/client/event/jms/MessageDeserializer.java 1483676 
  trunk/client/src/main/java/org/apache/oozie/client/event/messaging/SLAMessage.java PRE-CREATION 
  trunk/core/src/main/java/org/apache/oozie/event/messaging/MessageFactory.java 1483676 
  trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java PRE-CREATION 
  trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java 1483676 
  trunk/core/src/test/java/org/apache/oozie/jms/TestJMSSLAEventListener.java PRE-CREATION 

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


Testing
-------


Thanks,

Ryota Egashira

Re: Review Request: OOZIE-1296 SLA JMS Event Listener for publishing notifications related to SLA information

Posted by Rohini Palaniswamy <ro...@gmail.com>.

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



trunk/client/src/main/java/org/apache/oozie/client/event/SLAEvent.java
<https://reviews.apache.org/r/10226/#comment43108>

    Accidental change?



trunk/client/src/main/java/org/apache/oozie/client/event/jms/MessageDeserializer.java
<https://reviews.apache.org/r/10226/#comment43122>

    Make EventMessage have getJMSMessageProperties and setJMSMessageProperties and call these methods directly on that. Remove the setProperties method from JSONMessageDeserializer



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43135>

    Also include Long expectedDuration and actualDuration. You can send the expectedDuration in the message sent from the server. But the actualDuration you need not send from server and can just return actualEndTime - actualStartTime (after null checks) from the getActualDuration method. 



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43120>

    Remove this. Do not expose jobData in SLAMessage. If we put something in jobData, we will add methods later to SLAMessage reflecting what it is.



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43123>

    Move these getter and setter to EventMessage



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43121>

    Override this method, call super.setJMSMessageProperties and set the values here ( code from JSONMessageDeserializer.setProperties).



trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java
<https://reviews.apache.org/r/10226/#comment43124>

    Accidental change?



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43127>

    Do not declare constants again. Refer JMSJobEventListener constants



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43128>

    Remove the constructor



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43131>

    + messageBody



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43130>

    else {
      LOG.warn(No connection. Not sending message + messageBody
    }
    
    Same for JMSJobEventListener



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43132>

    Add a getTopic(appType, jobID) method to JMSTopicService and use that.



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43133>

    Why is this required?



trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java
<https://reviews.apache.org/r/10226/#comment43125>

    Class needs to be reverted. Mona had this change removed in the final patch for OOZIE-1244.



trunk/core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java
<https://reviews.apache.org/r/10226/#comment43126>

    Revert changes in all classes which make this change



trunk/core/src/test/java/org/apache/oozie/jms/TestJMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43134>

    Remove the try catch block in all the tests. Tests will fail with the exception automatically.


- Rohini Palaniswamy


On May 22, 2013, 12:45 a.m., Ryota Egashira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10226/
> -----------------------------------------------------------
> 
> (Updated May 22, 2013, 12:45 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Description
> -------
> 
> https://issues.apache.org/jira/browse/OOZIE-1296
> 
> note that this patch depends on OOZIE-1209, OOZIE-1234, which are not committed yet.
> doing modification on new classes created by OOZIE-1209, OOZIE-1234, (Event, SLAEvent,MessageFactory, JMSMessagingUtils, MessageDeserializer, etc), but since these classes not committed on trunk, RB doesn't take patch for only my diff part, thus, including entire class. please be aware.
> 
> still WIP, need to work on test case.
> 
> 
> This addresses bug OOZIE-1296.
>     https://issues.apache.org/jira/browse/OOZIE-1296
> 
> 
> Diffs
> -----
> 
>   trunk/client/src/main/java/org/apache/oozie/client/event/SLAEvent.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/JMSHeaderConstants.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/JSONMessageDeserializer.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/MessageDeserializer.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java PRE-CREATION 
>   trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/event/messaging/MessageFactory.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java PRE-CREATION 
>   trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/service/SLAService.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/jms/TestJMSSLAEventListener.java PRE-CREATION 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLAEventGeneration.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLARegistrationGetJPAExecutor.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLAService.java 1484974 
> 
> Diff: https://reviews.apache.org/r/10226/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ryota Egashira
> 
>

Re: Review Request: OOZIE-1296 SLA JMS Event Listener for publishing notifications related to SLA information

Posted by Rohini Palaniswamy <ro...@gmail.com>.

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



trunk/client/src/main/java/org/apache/oozie/client/event/message/EventMessage.java
<https://reviews.apache.org/r/10226/#comment43375>

    Do we need this? Can we remove the variable and getter method



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43374>

    Leave actual duration in millis only. In case of null, set to -1 instead of 0. 



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43377>

    Do we need this variable and it's getter, setters?. Should move the code to listener.



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43387>

    White space



trunk/client/src/main/java/org/apache/oozie/client/event/message/WorkflowJobMessage.java
<https://reviews.apache.org/r/10226/#comment43376>

    Why are these new imports required?



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43381>

    Constants from jmsjobeventlistener



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43383>

    Construct this map in this class only. It is server specific code and better not put in client facing class



trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java
<https://reviews.apache.org/r/10226/#comment43384>

    Small case parent



trunk/core/src/test/java/org/apache/oozie/jms/TestJMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43386>

    White space


- Rohini Palaniswamy


On May 24, 2013, 1:58 a.m., Ryota Egashira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10226/
> -----------------------------------------------------------
> 
> (Updated May 24, 2013, 1:58 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Description
> -------
> 
> https://issues.apache.org/jira/browse/OOZIE-1296
> 
> note that this patch depends on OOZIE-1209, OOZIE-1234, which are not committed yet.
> doing modification on new classes created by OOZIE-1209, OOZIE-1234, (Event, SLAEvent,MessageFactory, JMSMessagingUtils, MessageDeserializer, etc), but since these classes not committed on trunk, RB doesn't take patch for only my diff part, thus, including entire class. please be aware.
> 
> still WIP, need to work on test case.
> 
> 
> This addresses bug OOZIE-1296.
>     https://issues.apache.org/jira/browse/OOZIE-1296
> 
> 
> Diffs
> -----
> 
>   trunk/client/src/main/java/org/apache/oozie/client/event/SLAEvent.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/JMSHeaderConstants.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/JSONMessageDeserializer.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/MessageDeserializer.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/CoordinatorActionMessage.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/EventMessage.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/JobMessage.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java PRE-CREATION 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/WorkflowJobMessage.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/event/messaging/MessageFactory.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/jms/JMSJobEventListener.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java PRE-CREATION 
>   trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/service/SLAService.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/jms/TestJMSSLAEventListener.java PRE-CREATION 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLAEventGeneration.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLARegistrationGetJPAExecutor.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLAService.java 1484974 
> 
> Diff: https://reviews.apache.org/r/10226/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ryota Egashira
> 
>

Re: Review Request: OOZIE-1296 SLA JMS Event Listener for publishing notifications related to SLA information

Posted by Rohini Palaniswamy <ro...@gmail.com>.

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


Check the precommit build and fix the javadoc warning


trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43406>

    Remove this variable



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43409>

    Sorry. Expected is in mins in sla XML. Keep both in mins here and add that to javadoc for getters and setters.



trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java
<https://reviews.apache.org/r/10226/#comment43407>

    Repeated twice



trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java
<https://reviews.apache.org/r/10226/#comment43410>

    Add apache license header. There is a rat warning.


- Rohini Palaniswamy


On May 24, 2013, 8:17 a.m., Ryota Egashira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10226/
> -----------------------------------------------------------
> 
> (Updated May 24, 2013, 8:17 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Description
> -------
> 
> https://issues.apache.org/jira/browse/OOZIE-1296
> 
> note that this patch depends on OOZIE-1209, OOZIE-1234, which are not committed yet.
> doing modification on new classes created by OOZIE-1209, OOZIE-1234, (Event, SLAEvent,MessageFactory, JMSMessagingUtils, MessageDeserializer, etc), but since these classes not committed on trunk, RB doesn't take patch for only my diff part, thus, including entire class. please be aware.
> 
> still WIP, need to work on test case.
> 
> 
> This addresses bug OOZIE-1296.
>     https://issues.apache.org/jira/browse/OOZIE-1296
> 
> 
> Diffs
> -----
> 
>   trunk/client/src/main/java/org/apache/oozie/client/event/SLAEvent.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/JMSHeaderConstants.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/JSONMessageDeserializer.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/jms/MessageDeserializer.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/EventMessage.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/JobMessage.java 1484974 
>   trunk/client/src/main/java/org/apache/oozie/client/event/message/SLAMessage.java PRE-CREATION 
>   trunk/core/src/main/java/org/apache/oozie/client/rest/sla/JsonSLARegistrationEvent.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/event/messaging/MessageFactory.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/jms/JMSJobEventListener.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/jms/JMSSLAEventListener.java PRE-CREATION 
>   trunk/core/src/main/java/org/apache/oozie/service/JMSTopicService.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLACalcStatus.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLACalculatorMemory.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/SLASummaryBean.java 1484974 
>   trunk/core/src/main/java/org/apache/oozie/sla/service/SLAService.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/jms/TestJMSSLAEventListener.java PRE-CREATION 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLAEventGeneration.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLARegistrationGetJPAExecutor.java 1484974 
>   trunk/core/src/test/java/org/apache/oozie/sla/TestSLAService.java 1484974 
> 
> Diff: https://reviews.apache.org/r/10226/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ryota Egashira
> 
>