631233 - Python server returns records with a 'modified' value same as the 'newer' parameter

Status: RESOLVED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The following request:

http://sa.tk:5000/1.0/2d6xkt6ofzsb7xqqwxeffkm7wsqygorv/storage/history?full=1&newer=1296746835.09

Returns the following records:

DEBUG WBO:  {'sortindex': 2000, 'payload': ..., 'modified': Decimal('1296746835.09'), 'id': 'TBpVOawXJ2sN'}
DEBUG WBO:  {'sortindex': 100, 'payload': ..., 'modified': Decimal('1296746835.09'), 'id': '9_MnuCkjj5kU'}
DEBUG WBO:  {'sortindex': 100, 'payload': ..., 'modified': Decimal('1296746835.09'), 'id': 'peaB_KC_N4Hi'}

(The actual amount is 12 records. I did not show them all)

Expected result is only records that are newer than 1296746835.09

The above is the parsed JSON, but I have verified that the timestamps in the original response data are exactly the same.

Comment 1

[Tarek Ziadé (:tarek)]

Might be related to the rounding strategy. (round up/down)

Will add a test case and eventually fix it

Comment 2

[Tarek Ziadé (:tarek)]

Attachment: Correctly round the incoming timestamps

Comment 3

[Tarek Ziadé (:tarek)]

So... I found other spots where we're using float to represent time in the app. So the patch needs more work.

Working on this today -- Should have a patch sometime this CET afternoon.

Comment 4

[Tarek Ziadé (:tarek)]

Attachment: Patch to make sure round_time() raises a ValueError

We want to catch decimal-specific errror and have a valuerror, which is a more standard/global conversion error.

Comment 5

[Tarek Ziadé (:tarek)]

Attachment: Removed all float representation of time

Completes patch 5095594 - fixed the batch timestamp in the process

Comment 6

[Tarek Ziadé (:tarek)]

Attachment: Patch to make sure round_time() raises a ValueError

Comment 7

[Tarek Ziadé (:tarek)]

Forget about the first patch - <DELETE attachment 5095594> :)

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I'm not in the patch feedback list. But, I just appllied these patches and syncing works fine for me. The timestamps in the WBOs look good too. So I think think is all good.

Comment 9

[Toby Elliott [:telliott]]

Comment on attachment 509775 [details] [diff] [review]
Removed all float representation of time

Took me forever to figure out where request.server_time was coming from, but it's fine.

I would suggest that round_time take a value representing number of decimal places, only because I expect a function with the profile round_time(time) to give me an integer. That's just a naming thing and not a blocker.

Comment 10

[Tarek Ziadé (:tarek)]

(In reply to comment #9)
> Comment on attachment 509775 [details] [diff] [review]
> Removed all float representation of time
> 
> Took me forever to figure out where request.server_time was coming from, but
> it's fine.
> 
> I would suggest that round_time take a value representing number of decimal
> places, only because I expect a function with the profile round_time(time) to
> give me an integer. That's just a naming thing and not a blocker.

Sure, I can add a precision option.

Comment 11

[Tarek Ziadé (:tarek)]

Done in :
- http://hg.mozilla.org/services/server-core/rev/7a316f98b6ed
- http://hg.mozilla.org/services/server-storage/rev/0b8c9f1ab1c3