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