incubator-oozie-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Angelo K. Huang" <ange...@apache.org>
Subject Re: Review Request: OOZIE-580 use xml element to handle string escape when configure evaluator
Date Mon, 24 Oct 2011 23:55:39 GMT


> 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&amp;namespace=nova.proxy&amp;keyDBHash=Vsy6n_C7K6NG0z4R2eBlKg--]]></value>
>     </property>
> 
> 
> *** & has to convert to &amp; ***
> *** <![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&amp;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.
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message