Re: Review Request: XMLPS should be able to stream large results

[Ricky Nguyen <ri...@gmail.com>]

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

Ship it!


After testing I believe this is good to go.


trunk/grid/src/main/java/org/apache/oodt/grid/ProductQueryServlet.java
<https://reviews.apache.org/r/3543/#comment11427>

    I remember receiving partial responses. Also the servlet API says to flush the output stream to commit the response. This is especially needed since we don't know the length of the response.
    
    http://tomcat.apache.org/tomcat-5.5-doc/servletapi/javax/servlet/ServletResponse.html#getOutputStream()



trunk/grid/src/main/java/org/apache/oodt/grid/ProductQueryServlet.java
<https://reviews.apache.org/r/3543/#comment11428>

    Quoted from servlet API here http://tomcat.apache.org/tomcat-5.5-doc/servletapi/javax/servlet/http/HttpServlet.html#doGet(javax.servlet.http.HttpServletRequest,%20javax.servlet.http.HttpServletResponse):
    
    Where possible, set the Content-Length header (with the ServletResponse.setContentLength(int) method), to allow the servlet container to use a persistent connection to return its response to the client, improving performance. The content length is automatically set if the entire response fits inside the response buffer.
    
    When using HTTP 1.1 chunked encoding (which means that the response has a Transfer-Encoding header), do not set the Content-Length header.
    -----
    Thus, we don't set Content-Length and always use chunked Transfer-Encoding.
    
    Tested using cURL to watch the header values.
    Tomcat 5.5.35 : works
    Tomcat 6.0.35: works
    Tomcat 7.0.25: works



trunk/xmlps/src/main/java/org/apache/oodt/xmlps/product/DBMSExecutor.java
<https://reviews.apache.org/r/3543/#comment11425>

    it is organized. OODT first, JDK second, removed unused imports.



trunk/xmlps/src/main/java/org/apache/oodt/xmlps/product/XMLPSProductHandler.java
<https://reviews.apache.org/r/3543/#comment11426>

    yep organized.


- Ricky


On 2012-01-19 02:54:44, Ricky Nguyen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3543/
> -----------------------------------------------------------
> 
> (Updated 2012-01-19 02:54:44)
> 
> 
> Review request for oodt.
> 
> 
> Summary
> -------
> 
> See OODT-341: https://issues.apache.org/jira/browse/OODT-341
> 
> * CDEResult extends org.apache.oodt.xmlquery.Result
> * CDEResult mimetype is always "text/plain"
> * CDEResult size is always 0
> * CDEResult inputstream is CDEResultInputStream
> * CDEResultInputStream has IO methods and wraps a CDEResult
> * CDEResult wraps a ResultSet and returns rows as Strings, applying MappingFuncs if a Mapping is provided, and appending constant fields if a List of CDEValues is provided.
> * ProductQueryServlet relies on servlet container to handle Content-Length (and possibly Transfer-Encoding: chunked)
> 
> 
> This addresses bug OODT-341.
>     https://issues.apache.org/jira/browse/OODT-341
> 
> 
> Diffs
> -----
> 
>   trunk/grid/src/main/java/org/apache/oodt/grid/ProductQueryServlet.java 1183564 
>   trunk/xmlps/pom.xml 1233127 
>   trunk/xmlps/src/main/java/org/apache/oodt/xmlps/product/DBMSExecutor.java 1233127 
>   trunk/xmlps/src/main/java/org/apache/oodt/xmlps/product/XMLPSProductHandler.java 1233127 
>   trunk/xmlps/src/main/java/org/apache/oodt/xmlps/structs/CDEResult.java 1233127 
>   trunk/xmlps/src/main/java/org/apache/oodt/xmlps/structs/CDEResultInputStream.java PRE-CREATION 
>   trunk/xmlps/src/test/java/org/apache/oodt/xmlps/structs/TestCDEResult.java PRE-CREATION 
>   trunk/xmlps/src/test/java/org/apache/oodt/xmlps/structs/TestCDEResultInputStream.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/3543/diff
> 
> 
> Testing
> -------
> 
> Runs in Tomcat 7. I've used it at CHLA.
> NOT tested in other app servers. Which other app servers should I test?
> NOT tested MappingFuncs. Is it necessary?
> Added 2 unit tests: TestCDEResult and TestCDEResultInputStream, which both pass.
> 
> 
> Thanks,
> 
> Ricky
> 
>