Ignore bad date from remote server

We run into a case that remote server would return date that is off by 30 minutes when fetching data.
This then cause our server to fetch the resource every time.
(the resource was still served to user and cached, but on next retrieval from cache it was dropped and fetched again).

So the fix is to compare the date on the response and if too old (configured) it will be ignored and shindig server time will be used.

[johnfargo, 2010-07-30 01:00:07]

Looks like the patch got out of sync. svn up and repatch. thx!

http://codereview.appspot.com/1689060/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/1689060/diff/1/3#newcode131
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:131:
static final long DEFAULT_DRIFT_LIMIT = 3L * 60L * 1000L;
mark this as _MS for millis, w/ comment.

http://codereview.appspot.com/1689060/diff/1/3#newcode147
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:147:
@Inject(optional = true) @Named("shindig.http.date-drift-limit")
same "-ms"

[zhoresh, 2010-07-30 01:23:14]

Sync to head and fix according to comments

[johnfargo, 2010-07-30 02:20:49]

http://codereview.appspot.com/1689060/diff/10001/5003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/1689060/diff/10001/5003#newcode460
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:460:
if (timestamp < (currentTime - responseDateDriftLimit)) {
this logic presumes that we're protecting only against clocks that are behind.
why not ahead?

if (responseDateDriftLimit > Math.abs(currentTime - timestamp)) { ... }

[zhoresh, 2010-07-30 02:26:49]

I only experience the server legging behind, but you right should check both
way, ptal

[johnfargo, 2010-07-30 03:24:56]

LGTM in general. Would be nice to see both greater-than-limit and
less-than-limit tests.

http://codereview.appspot.com/1689060/diff/15001/16002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
(right):

http://codereview.appspot.com/1689060/diff/15001/16002#newcode460
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:460:
if (responseDateDriftLimit < Math.abs(currentTime - timestamp)) {
don't you want > here? :)

[zhoresh, 2010-07-30 06:06:14]

On Thu, Jul 29, 2010 at 8:24 PM, <johnfargo@gmail.com> wrote:

> LGTM in general. Would be nice to see both greater-than-limit and
> less-than-limit tests.
>
Will add the test

>
>
> http://codereview.appspot.com/1689060/diff/15001/16002
>
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
> (right):
>
> http://codereview.appspot.com/1689060/diff/15001/16002#newcode460
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:460:
> if (responseDateDriftLimit < Math.abs(currentTime - timestamp)) {
> don't you want > here? :)

Nope, we want to use shindig date if the time is off by more then the
allowed drift. (and the test should prove it).
Maybe if I will reverse the order it will make more sense: if (Diff >
allowed drift) then ignore response time.



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

[johnfargo, 2010-07-30 06:25:02]

Yep, oops, just misread the test there. Looks good.

On Thu, Jul 29, 2010 at 11:05 PM, Ziv Horesh <zhoresh@gmail.com> wrote:

>
> On Thu, Jul 29, 2010 at 8:24 PM, <johnfargo@gmail.com> wrote:
>
>> LGTM in general. Would be nice to see both greater-than-limit and
>> less-than-limit tests.
>>
> Will add the test
>
>>
>>
>> http://codereview.appspot.com/1689060/diff/15001/16002
>>
>> File
>>
>> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java
>> (right):
>>
>> http://codereview.appspot.com/1689060/diff/15001/16002#newcode460
>>
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:460:
>> if (responseDateDriftLimit < Math.abs(currentTime - timestamp)) {
>> don't you want > here? :)
>
> Nope, we want to use shindig date if the time is off by more then the
> allowed drift. (and the test should prove it).
> Maybe if I will reverse the order it will make more sense: if (Diff >
> allowed drift) then ignore response time.
>
>
>
>>
>> http://codereview.appspot.com/1689060/show
>>
>
>