Re: For consideration: base APIs supporting streaming fetch (issue1626044)

[zh...@gmail.com]

And don't forget to write tests...


http://codereview.appspot.com/1626044/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/1626044/diff/5001/6002#newcode498
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:498:
if (body == null || !(body instanceof
MutableContent.ByteArrayContentBytes)) {
I would postpond detection to the time you need it.
The encoding is really needed only in the case of batch request, so at
that point you should have all data.

http://codereview.appspot.com/1626044/diff/5001/6003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
(right):

http://codereview.appspot.com/1626044/diff/5001/6003#newcode296
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:296:
bufferedStream = IOUtils.toByteArray(getStream());
What if someone already ask to get the stream?

http://codereview.appspot.com/1626044/diff/5001/6003#newcode301
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:301:
return bufferedStream;
Set streamReturned to true

http://codereview.appspot.com/1626044/diff/5001/6003#newcode314
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:314:
return stream;
Set streamReturned to true

http://codereview.appspot.com/1626044/show

Re: For consideration: base APIs supporting streaming fetch (issue1626044)

[John Hjelmstad <jo...@gmail.com>]

Thanks for the feedback, Ziv! Comments inline.

On Mon, Jun 14, 2010 at 4:56 PM, <zh...@gmail.com> wrote:

> And don't forget to write tests...
>
>
> http://codereview.appspot.com/1626044/diff/5001/6002
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
> (right):
>
> http://codereview.appspot.com/1626044/diff/5001/6002#newcode498
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:498:
> if (body == null || !(body instanceof
> MutableContent.ByteArrayContentBytes)) {
> I would postpond detection to the time you need it.
> The encoding is really needed only in the case of batch request, so at
> that point you should have all data.
>

It's actually "needed" in one critically important other way -- as a side
effect of setting the Content-Type for the HttpResponse object. About to
upload a patch that maintains these semantics while lazy-parsing the
encoding when needed.


>
> http://codereview.appspot.com/1626044/diff/5001/6003
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java
> (right):
>
> http://codereview.appspot.com/1626044/diff/5001/6003#newcode296
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:296:
> bufferedStream = IOUtils.toByteArray(getStream());
> What if someone already ask to get the stream?
>

Barf, by design :)


>
> http://codereview.appspot.com/1626044/diff/5001/6003#newcode301
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:301:
> return bufferedStream;
> Set streamReturned to true
>

At this point, it's guaranteed to already be set to true.


>
> http://codereview.appspot.com/1626044/diff/5001/6003#newcode314
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:314:
> return stream;
> Set streamReturned to true
>

Done.


>
> http://codereview.appspot.com/1626044/show
>