> On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 369 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line369> > > > > What is the purpose of this utility method? Is it something that can be > > used more generally, i.e., across test cases
There is addRecordToWfJobTable() at XDataTestcase. This one is mainly for this test case. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 379 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line379> > > > > If this is a real host name, then it should be anonymized. fixed it. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 380 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line380> > > > > The host name should be anonymized if its a real host name fixed it. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 393 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line393> > > > > Will this line of code be executed? I was not sure about the semantics > > of fail. fixed it. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 411 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line411> > > > > Will this line of code be executed? I was not sure about the semantics > > of fail. fixed it. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 416 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line416> > > > > Will this line of code be executed? I was not sure about the semantics > > of fail. I think u mean javadoc. added it. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 398 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line398> > > > > Can you please document the purpose of this method? added it. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java, > > line 177 > > <https://reviews.apache.org/r/2427/diff/3/?file=52892#file52892line177> > > > > What is the intention of this test case? Could you please add a top > > level comment for the test case and comments in the test case to describe > > the logic and the criteria being evaluated? added it. > On 2011-10-24 20:47:37, Santhosh Srinivasan wrote: > > /trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java, line 66 > > <https://reviews.apache.org/r/2427/diff/3/?file=52890#file52890line66> > > > > Interesting. Your comment indicates that it should not happen. It could > > happen and the code will try to alleviate this problem by escaping special > > characters. The escaping may or may not solve the problem since the > > JDOMException could be for a range of issues. fixed it. - Angelo K. ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2427/#review2803 ----------------------------------------------------------- On 2011-10-24 20:02:05, Angelo K. Huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2427/ > ----------------------------------------------------------- > > (Updated 2011-10-24 20:02:05) > > > Review request for oozie. > > > Summary > ------- > > try { > String valueElem = "<value>"+value+"</value>"; > XmlUtils.parseXml(valueElem); > } > catch (JDOMException ex) { > // It should not happen, so escape the characters for xml > value = XmlUtils.escapeCharsForXML(value); > } > > With above check, for element CDATA can be avoided for escaping, EX: > > For these two elements, first one has to convert because of '&', however > second one can be avoided. > > <property> > <name>test.ampsign</name> > > <value>http://app1.soln-stage.nova.cp.vip.ne1.yahoo.com/nova-webservices?urlSigner=signUrl&namespace=nova.proxy</value> > </property> > <property> > <name>test.cdata</name> > > <value><![CDATA[?redirect=http%3A%2F%2Fapp1.soln-stage.nova.cp.vip.ne1.yahoo.com%3A4080%2Fnova-webservices%2Fv1%2FurlSigner%2FsignUrl&namespace=nova.proxy&keyDBHash=Vsy6n_C7K6NG0z4R2eBlKg--]]></value> > </property> > > > *** & has to convert to & *** > *** <![CDATA[]] does not need to convert. *** > > <property> > <name>test.ampsign</name> > > <value>http://app1.soln-stage.nova.cp.vip.ne1.yahoo.com/nova-webservices?urlSigner=signUrl&namespace=nova.proxy</value> > </property> > <property> > <name>test.cdata</name> > > <value><![CDATA[?redirect=http%3A%2F%2Fapp1.soln-stage.nova.cp.vip.ne1.yahoo.com%3A4080%2Fnova-webservices%2Fv1%2FurlSigner%2FsignUrl&namespace=nova.proxy&keyDBHash=Vsy6n_C7K6NG0z4R2eBlKg--]]></value> > </property> > > > This addresses bug OOZIE-580. > https://issues.apache.org/jira/browse/OOZIE-580 > > > Diffs > ----- > > /trunk/core/src/main/java/org/apache/oozie/DagELFunctions.java 1185461 > > /trunk/core/src/main/java/org/apache/oozie/command/wf/ActionStartXCommand.java > 1185461 > > /trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionStartXCommand.java > 1185461 > /trunk/release-log.txt 1185461 > > Diff: https://reviews.apache.org/r/2427/diff > > > Testing > ------- > > > Thanks, > > Angelo K. > >
