You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Robert Kanter <rk...@cloudera.com> on 2015/11/06 00:26:36 UTC
Re: Review Request 36123: OOZIE-2259 : Callback Action Executor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36123/#review105324
-----------------------------------------------------------
client/src/main/resources/callback-action-0.1.xsd (line 30)
<https://reviews.apache.org/r/36123/#comment163850>
Shouldn't we allow minOccurs="0" here? What if there are no arguments for the callback? Or it's somehow encoded in the URL or whatever.
core/src/main/java/org/apache/oozie/action/callback/CallBackActionExecutor.java (line 64)
<https://reviews.apache.org/r/36123/#comment163854>
This is probably too much for this JIRA, so maybe it should go in another JIRA, but it would be nice if the METHOD could somehow be pluggable. For example, say a user has a custom notificaiton system or uses something other than HTTP or KMS, then they could implement a little bit of code, add a new METHOD, and use that. We don't have any existing action types that are themselves pluggable, and doing that sounds pretty complicated. In any case, can you file a JIRA to add this ability? (You can leave it unassigned)
core/src/main/java/org/apache/oozie/action/callback/CallBackActionExecutor.java (line 265)
<https://reviews.apache.org/r/36123/#comment163863>
Perhaps we should implement kill? Let's say the timeout is set really long and it's taking a long time to send the callback, the user might want to kill the action. I'm not sure if there's a way to "interrupt" an HTTP call or JMS call. If not, or if this would be super complicated, then don't worry about it.
core/src/main/java/org/apache/oozie/service/CallBackActionService.java (line 59)
<https://reviews.apache.org/r/36123/#comment163867>
Have you tried using a self-signed certificate? In general, you should have to pass "-Djavax.net.ssl.trustStore=/path/to/trustore.jks" to the Oozie Server Tomcat instance.
core/src/main/resources/oozie-default.xml (line 2577)
<https://reviews.apache.org/r/36123/#comment163866>
This is specific for the CallBackAction, right? Perhaps we should rename this to "oozie.action.callback.jms.naming.factory.initial".
And the description should say "CallBack Action", not "jms action".
core/src/main/resources/oozie-default.xml (line 2588)
<https://reviews.apache.org/r/36123/#comment163868>
Better description
core/src/main/resources/oozie-default.xml (line 2596)
<https://reviews.apache.org/r/36123/#comment163869>
Better description
core/src/main/resources/oozie-default.xml (line 2604)
<https://reviews.apache.org/r/36123/#comment163870>
Better description
core/src/main/resources/oozie-default.xml (line 2612)
<https://reviews.apache.org/r/36123/#comment163871>
Better description
core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java (line 60)
<https://reviews.apache.org/r/36123/#comment163872>
Will this work with port "0" (i.e. a random open port)? Otherwise, this could run into port conflicts.
core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java (line 65)
<https://reviews.apache.org/r/36123/#comment163873>
Same. We should use port "0".
core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java (line 138)
<https://reviews.apache.org/r/36123/#comment163876>
There should be a call to ``fail("reason")`` in the try block so that if there's no Exception, the test will fail.
core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java (line 175)
<https://reviews.apache.org/r/36123/#comment163874>
There should be a call to ``fail("reason")`` in the try block so that if there's no Exception, the test will fail.
core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java (line 194)
<https://reviews.apache.org/r/36123/#comment163875>
There should be a call to ``fail("reason")`` in the try block so that if there's no Exception, the test will fail.
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 7)
<https://reviews.apache.org/r/36123/#comment163877>
We should be consistent about the capitalization of "Callback". Is it "CallBack" or "Callback"? Personally, I prefer "Callback". Other parts in the code and elsewhere had "CallBack".
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 12)
<https://reviews.apache.org/r/36123/#comment163878>
3.2.4?
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 15)
<https://reviews.apache.org/r/36123/#comment163879>
=http(s)=
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 23)
<https://reviews.apache.org/r/36123/#comment163880>
Let's use a newer version of the workflow schema, (e.g. 0.4 or 0.5)
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 26)
<https://reviews.apache.org/r/36123/#comment163881>
The xmlns here is wrong
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 46)
<https://reviews.apache.org/r/36123/#comment163882>
http(s)
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 52)
<https://reviews.apache.org/r/36123/#comment163883>
You should also list the property names that these get populated in.
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 54)
<https://reviews.apache.org/r/36123/#comment163884>
No need to repeat the oozie-site/default properties here.
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 105)
<https://reviews.apache.org/r/36123/#comment163886>
workflow schema 0.4 or 0.5
docs/src/site/twiki/DG_CallbackActionExtension.twiki (line 108)
<https://reviews.apache.org/r/36123/#comment163885>
xmlns is wrong
- Robert Kanter
On Aug. 12, 2015, 11:36 a.m., Jaydeep Vishwakarma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36123/
> -----------------------------------------------------------
>
> (Updated Aug. 12, 2015, 11:36 a.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2259
> https://issues.apache.org/jira/browse/OOZIE-2259
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Adding callback as an action, It have support for HTTP and JMS server. Not covering Excecutor level queue, I will create a separate jira for it.
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/oozie/cli/OozieCLI.java 48bac7d
> client/src/main/resources/callback-action-0.1.xsd PRE-CREATION
> core/src/main/java/org/apache/oozie/action/callback/CallBackActionExecutor.java PRE-CREATION
> core/src/main/java/org/apache/oozie/action/callback/JMSNotification.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/CallBackActionService.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 0a7e250
> core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java PRE-CREATION
> docs/src/site/twiki/DG_CallbackActionExtension.twiki PRE-CREATION
> docs/src/site/twiki/DG_CommandLineTool.twiki 1823247
> docs/src/site/twiki/index.twiki 8591530
>
> Diff: https://reviews.apache.org/r/36123/diff/
>
>
> Testing
> -------
>
> Done,
> Will add more test cases.
>
>
> Thanks,
>
> Jaydeep Vishwakarma
>
>
Re: Review Request 36123: OOZIE-2259 : Callback Action Executor
Posted by Jaydeep Vishwakarma <ja...@gmail.com>.
> On Nov. 5, 2015, 11:26 p.m., Robert Kanter wrote:
> > client/src/main/resources/callback-action-0.1.xsd, line 30
> > <https://reviews.apache.org/r/36123/diff/5/?file=1038361#file1038361line30>
> >
> > Shouldn't we allow minOccurs="0" here? What if there are no arguments for the callback? Or it's somehow encoded in the URL or whatever.
Yes, Puru also mentioned the same. I am taking care of it.
> On Nov. 5, 2015, 11:26 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/action/callback/CallBackActionExecutor.java, line 64
> > <https://reviews.apache.org/r/36123/diff/5/?file=1038362#file1038362line64>
> >
> > This is probably too much for this JIRA, so maybe it should go in another JIRA, but it would be nice if the METHOD could somehow be pluggable. For example, say a user has a custom notificaiton system or uses something other than HTTP or KMS, then they could implement a little bit of code, add a new METHOD, and use that. We don't have any existing action types that are themselves pluggable, and doing that sounds pretty complicated. In any case, can you file a JIRA to add this ability? (You can leave it unassigned)
Thats nice thought. I will create the jira.
> On Nov. 5, 2015, 11:26 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/action/callback/CallBackActionExecutor.java, line 265
> > <https://reviews.apache.org/r/36123/diff/5/?file=1038362#file1038362line265>
> >
> > Perhaps we should implement kill? Let's say the timeout is set really long and it's taking a long time to send the callback, the user might want to kill the action. I'm not sure if there's a way to "interrupt" an HTTP call or JMS call. If not, or if this would be super complicated, then don't worry about it.
We can abort the http calls. By nature of JMS is asynchronous. Producer publishes messages, it doesn't need to wait consumer. I will handle this case in https://issues.apache.org/jira/browse/OOZIE-2331.
> On Nov. 5, 2015, 11:26 p.m., Robert Kanter wrote:
> > core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java, line 60
> > <https://reviews.apache.org/r/36123/diff/5/?file=1038366#file1038366line60>
> >
> > Will this work with port "0" (i.e. a random open port)? Otherwise, this could run into port conflicts.
Agreed with port conflict. Searching for available port and assigning it.
> On Nov. 5, 2015, 11:26 p.m., Robert Kanter wrote:
> > core/src/test/java/org/apache/oozie/action/callback/TestCallBackActionExecutor.java, line 65
> > <https://reviews.apache.org/r/36123/diff/5/?file=1038366#file1038366line65>
> >
> > Same. We should use port "0".
Searching for random port and assigning it .
> On Nov. 5, 2015, 11:26 p.m., Robert Kanter wrote:
> > docs/src/site/twiki/DG_CallbackActionExtension.twiki, line 7
> > <https://reviews.apache.org/r/36123/diff/5/?file=1038367#file1038367line7>
> >
> > We should be consistent about the capitalization of "Callback". Is it "CallBack" or "Callback"? Personally, I prefer "Callback". Other parts in the code and elsewhere had "CallBack".
Yeah "Callback" make more sense.
> On Nov. 5, 2015, 11:26 p.m., Robert Kanter wrote:
> > core/src/main/java/org/apache/oozie/service/CallBackActionService.java, line 59
> > <https://reviews.apache.org/r/36123/diff/5/?file=1038364#file1038364line59>
> >
> > Have you tried using a self-signed certificate? In general, you should have to pass "-Djavax.net.ssl.trustStore=/path/to/trustore.jks" to the Oozie Server Tomcat instance.
I have checked it , It is working. I am thinking to keep the trust store separate from server, As everytime we add the trust store you need a restart. Passing it throght workflow will make more sense, I can take this in another jira , what do you say?
- Jaydeep
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36123/#review105324
-----------------------------------------------------------
On Nov. 12, 2015, 12:21 p.m., Jaydeep Vishwakarma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36123/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2015, 12:21 p.m.)
>
>
> Review request for oozie.
>
>
> Bugs: OOZIE-2259
> https://issues.apache.org/jira/browse/OOZIE-2259
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Adding callback as an action, It have support for HTTP and JMS server. Not covering Excecutor level queue, I will create a separate jira for it.
>
>
> Diffs
> -----
>
> client/src/main/java/org/apache/oozie/cli/OozieCLI.java 48bac7d
> client/src/main/resources/callback-action-0.1.xsd PRE-CREATION
> core/src/main/java/org/apache/oozie/action/callback/CallbackActionExecutor.java PRE-CREATION
> core/src/main/java/org/apache/oozie/action/callback/JMSNotification.java PRE-CREATION
> core/src/main/java/org/apache/oozie/service/CallbackActionService.java PRE-CREATION
> core/src/main/resources/oozie-default.xml 0a7e250
> core/src/test/java/org/apache/oozie/action/callback/TestCallbackActionExecutor.java PRE-CREATION
> docs/src/site/twiki/DG_CallbackActionExtension.twiki PRE-CREATION
> docs/src/site/twiki/DG_CommandLineTool.twiki 1823247
> docs/src/site/twiki/index.twiki 8591530
>
> Diff: https://reviews.apache.org/r/36123/diff/
>
>
> Testing
> -------
>
> Done,
> Will add more test cases.
>
>
> Thanks,
>
> Jaydeep Vishwakarma
>
>