787871 - Remove the X-Timestamp header from Sync2.0

Status: RESOLVED WONTFIX

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

Description

[Ryan Kelly [:rfkelly]]

Attachment: patch to remove X-Timestamp header from sync2.0

Seriously, if client software is using X-Timestamp for anything then it's probably doing something wrong.  The only justification I can see is to include it in client-side logs to make debugging easier.  So we should either remove it, or put a big warning in the docs that this is what it is designed for.

Comment 1

[Toby Elliott [:telliott]]

Comment on attachment 657775 [details] [diff] [review]
patch to remove X-Timestamp header from sync2.0

r+ with the huge caveat that this is a client call to make. We initially added this as helper data because the client wanted to know server timestamps and it made some debugging easier. If they aren't using it, then removing it from the protocol is fine from my perspective.

Comment 2

[Gregory Szorc [:gps]]

Don't commit this until we have a story in bug 787870.

Comment 3

[Ryan Kelly [:rfkelly]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Don't commit this until we have a story in bug 787870.

Yep.  Happy to keep it as long as we have something to put in the docs describing what it's for.

Comment 4

[Ryan Kelly [:rfkelly]]

Adding :rnewman and noting the req from Bug 787870 for completeness:  clients may use this header to help correct for clock drift, allowing more accurate resolution of conflicting writes from different clients.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 657775 [details] [diff] [review]
patch to remove X-Timestamp header from sync2.0

Review of attachment 657775 [details] [diff] [review]:
-----------------------------------------------------------------

I defer to Richard on this one.

Comment 6

[Richard Newman [:rnewman]]

To recap, there are three valid uses of timestamps on the client:

* Logging
* Clock drift correction
* Detecting violated invariants (e.g., server clock jumps).

I think logging alone is valuable; the combination of local and server timestamps could tell us if a client's clock is wrong, for example. And without that header it's impossible to write certain tests (upload, download, check that the timestamps correlate). 

(Given that we don't yet correct for clock drift in the client, I'm happy to accord that little weight, but it's a nice-to-have.)

To your point in Comment 0: the ability for the client to do most dumb things is eliminated by the shift of info/collections and headers into version numbers.

It's simply no longer possible to write a client that does anything more than the three things above, because the only two occurrences of timestamps are in X-Timestamp and inside records -- they are only ever passed server-to-client.

So the question becomes: is it better to save the bytes, or enable those three things on the client? Phrased that way, I'm generally against stripping out this header.

My suggestion is to briefly summarize this -- that timestamps pass only from server to client, that the only acceptable uses are in detecting and handling clock-related errors and in logging -- and perhaps indicate that clients should not rely on the presence of the header.

Comment 7

[Ryan Kelly [:rfkelly]]

Agreed on all counts, esp with Bug 787870 basically sorted.  The header stays, I'll add some notes in the docs about how it should be used.

Comment 8

[Gregory Szorc [:gps]]

So, I just remembered the Date response header is mandated by RFC 2616. This provides 1s resolution of server time. Now that we've demoted timestamps to be a "fallback" reconciling mechanism (bug 787870), I don't believe the extra resolution is warranted. Therefore, I'm having a hard time justifying the existence of X-Timestamp *in addition to* Date.

Comment 9

[Ryan Kelly [:rfkelly]]

We currently have a guarantee that the value in X-Timestamp is equal to the new "timestamp" field of any BSOs modified by the request.  If we ditch it in favour of Date then we should modify the precision of the "timestamp" field to match.

Comment 10

[Ryan Kelly [:rfkelly]]

I'm going to close this out, if anyone feels strongly about removing it then please re-open.  We will also revisit the resolution of these timestamps as part of Bug 778994.