Move the drift time correction from http response to pipeline

The time drift correction should only apply when fetching resource not for any creation of HttpResponse

[johnfargo, 2010-11-11 00:59:21]

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:162:
public static HttpResponse fixDriftTime(HttpResponse response) {
1. Please document the algorithm in JavaDoc on the method.
2. Nit: name maybeFixDriftTime() to underscore that doing so is conditional.

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java
(right):

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java:57:
HttpResponse.setTimeSource(timeSource);
Hm, wonder if there's any risk in this (non-deterministically) breaking other
tests that run concurrently..

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java:280:
}
would be nice to get a higher-level unit test for the overall
DefaultRequestPipeline exhibiting time-fixing behavior.

[zhoresh, 2010-11-11 18:13:59]

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:162:
public static HttpResponse fixDriftTime(HttpResponse response) {
On 2010/11/11 00:59:21, johnfargo wrote:
> 1. Please document the algorithm in JavaDoc on the method.
> 2. Nit: name maybeFixDriftTime() to underscore that doing so is conditional.

Done.

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java
(right):

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java:57:
HttpResponse.setTimeSource(timeSource);
On 2010/11/11 00:59:21, johnfargo wrote:
> Hm, wonder if there's any risk in this (non-deterministically) breaking other
> tests that run concurrently..

Test passed when I run them, but I see your concern.
I changed it to use the same time source as the HttpResponseTest. There is still
risk if any of the functions actually updating time.

http://codereview.appspot.com/2993042/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultRequestPipelineTest.java:280:
}
On 2010/11/11 00:59:21, johnfargo wrote:
> would be nice to get a higher-level unit test for the overall
> DefaultRequestPipeline exhibiting time-fixing behavior.

Done.

[zhoresh, 2010-11-11 18:14:30]

Update according to Fargo comments

[johnfargo, 2010-11-11 18:19:00]

LGTM

The static time thing is something we should keep an eye on, but let's cross
that bridge when we get to it.

On Thu, Nov 11, 2010 at 10:14 AM, <zhoresh@gmail.com> wrote:

> Update according to Fargo comments
>
>
> http://codereview.appspot.com/2993042/
>

[zhoresh, 2010-11-11 19:00:11]

I gues at this point the advantage of the time source is that it is
consistent, which improve calculation.
But it is still not great if you want a moving time.
I tried to set time for just the request pipeline, but that doesn't work
because the pipeline internally use HttpResponse which use different time.

Change submitted @r1034046


On Thu, Nov 11, 2010 at 10:18 AM, John Hjelmstad <johnfargo@gmail.com>wrote:

> LGTM
>
> The static time thing is something we should keep an eye on, but let's
> cross that bridge when we get to it.
>
>
> On Thu, Nov 11, 2010 at 10:14 AM, <zhoresh@gmail.com> wrote:
>
>> Update according to Fargo comments
>>
>>
>> http://codereview.appspot.com/2993042/
>>
>
>