Closed Bug 1250189 Opened 8 years ago Closed 6 years ago

[meta] Support atomic write of large batches of items

Categories

(Firefox :: Sync, defect, P3)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: rnewman, Assigned: rfkelly)

References

Details

(Keywords: meta, Whiteboard: [sync-atomic])

Attachments

(1 file)

docs PR adding proposed batch functionality
54 bytes, patch
rnewman
: feedback+
markh
: feedback+
Details | Diff | Splinter Review 
Sync suffers from two kinds of partial-state bugs:

* Partial reads, where a client downloads some records that reflect an inconsistent state (typically because of interruption)

* Partial writes, where a client is interrupted, the server becomes unavailable, or another client interferes (if the uploading client uses X-I-U-S) before uploading all of the changes it wants to upload.


It's possible for the client to make itself resilient against partial reads: buffer downloaded records and check for consistency, retry downloads as needed, etc.

It's intrinsically not possible for the client to resist partial writes: the server 'commits' each uploaded batch immediately, and there's a limit to the size of a batch. 

Worse, because partial writes leave the server in an inconsistent state, a client that's robust against partial reads _can leave itself unable to sync_! That's not good.


In order to make bookmark sync work correctly, particularly on platforms like iOS that refuse to process inconsistent server states, we need server support for atomic writes.

We've discussed transactions and various other options in the past, for years, and haven't committed to an implementation approach. I think I have a backwards-compatible option.


The fundamental primitive is atomic application of changes from one collection to another.

A client can then upload multiple batches of records to a new, private, collection, and finally apply those batches in a single operation.

We can respect X-I-U-S on that application, which gives us everything we need.

Existing clients will continue to write directly to the main collection; nothing in the existing API changes, which was our concern about transaction support.


For efficiency's sake we'd want to extend this a little:

* Creation of a temporary collection is managed by the server, so we can distinguish these from real collections. This creation step returns the same status codes as a normal write, including Conflict Detected.

* Temporary collections expire after a reasonable amount of time (two hours?).

* Writes to the temporary collection are rejected with a conflict if the original collection has been modified since creation. This allows clients to fail faster, saving bandwidth.

* The merge operation fails with the same status codes, also.

So rather than a client doing:


    POST /storage/bookmarks
>>> POST /storage/bookmarks
>>> POST /storage/bookmarks

it would:

    POST /storage/bookmarks/write
>>> POST /storage/bookmarks.1234
>>> POST /storage/bookmarks.1234
>>> POST /storage/bookmarks.1234?commit=true


The client-side storage client wouldn't need much extension to support this, but it would buy us the atomicity that we need.

Comments and counter-proposals?
Flags: needinfo?(rfkelly)
Flags: needinfo?(dcoates)
My first impression is that this looks great! I'd love to start working on improving our sync data consistency in Q2, so now's a good time to start thinking about this. I'll give this a deeper think this week.
Flags: needinfo?(dcoates)
Wonderful!

Something to noodle on: is there anything we can get done and deployed within the next four weeks? The sooner this gets out, the less likelihood of an iOS client stranding itself.

E.g., a single operation to insert into a collection by reading records from another, with none of the other neat server support?
Sounds solid and backwards-compatible; from an API perspective I think you might be better off doing it in a separate namespace rather than dangling additional bits off of the existing storage API.  Consider, as a strawman:

>>> POST /batch-upload/bookmarks             ; start a new batch upload
>>> POST /batch-upload/bookmarks/1234        ; add your new items to the batch
>>> POST /batch-upload/bookmarks/1234        ; and some more items
>>> POST /batch-upload/bookmarks/1234/commit ; write them back atomically

Or do you need other collection-like operations on it?  For example, would you expect "temporary collections" to show up in /info/collections?
Flags: needinfo?(rfkelly)
(In reply to Ryan Kelly [:rfkelly] from comment #3)

> Or do you need other collection-like operations on it?  For example, would
> you expect "temporary collections" to show up in /info/collections?

For our current purposes, no: just writes and a final commit.

Hanging it off the real collection URL has a couple of small benefits:

* Simplifies clients that are built to work relative to a base /storage/ URL, which is all of them. This just acts like another writable collection.
* Feels a little more RESTy: you're manipulating that storage resource. (And this helps imply the XIUS linkage, same response codes, etc.)

If it's more costly to build or has other downsides, then prioritize those things accordingly -- clients can adapt :D
Just so I understand the situation better, what are the consequences of this *not* happening in the next four weeks? What do you mean by "stranded" and will clients be (more) broken than they are today by not adding an atomic api? Also can you elaborate on this comment:

> Worse, because partial writes leave the server in an inconsistent state, a client that's robust against partial reads _can leave itself unable to sync_! That's not good.

Is this the case with the current ios client?
Flags: needinfo?(rnewman)
(In reply to Danny Coates [:dcoates] from comment #5)
> Just so I understand the situation better, what are the consequences of this
> *not* happening in the next four weeks? What do you mean by "stranded" and
> will clients be (more) broken than they are today by not adding an atomic
> api?

Stranded means that clients that refuse to handle inconsistent/corrupt server states (i.e., iOS) won't ever sync again until the server state is corrected, because no other clients will completely fix up the server.

(Current Android and desktop clients are blindly trusting, and settle for simply screwing up your bookmarks instead.)

The key to not stranding clients is to make sure that clients -- other and same -- don't inadvertently corrupt the server. This bug is the only way to do that in the general case; we can mitigate by uploading in larger chunks, which buys us a hacky kind of atomicity, but clients that exceed the 2MB upload limit are unable to avoid the danger.

If we don't get something like this out the door by the time iOS ships, there's some risk that clients will get into a state that requires manual intervention.

My hope is that the risk is low because iOS clients rarely need to upload lots and lots of records, but I don't have statistics either way.

If we can't get atomic writes in time, we'll take one of two avenues:

* Always upload a single batch. The server will reject batches that are too big; those users will burn 2+MB per sync, a few times a day, until we ship a new version of the client that doesn't, or we allow the server to accept larger batches. But at least the server contents will remain consistent!

* Upload large batches, and hope that users whose data span multiple batches don't get interrupted.

And we'll also attempt to retry some uploads, just in case.


> > Worse, because partial writes leave the server in an inconsistent state, a client that's robust against partial reads _can leave itself unable to sync_! That's not good.
> 
> Is this the case with the current ios client?

Yes, though "current" means "ships for the first time in four weeks". If the iOS client upload is interrupted, it can -- just like any other client -- leave the server in a partially updated state, and it'll subsequently refuse to sync the mess it made.
Flags: needinfo?(rnewman)
See Also: → 1250561
Blocks: 1196238
> Stranded means that clients that refuse to handle inconsistent/corrupt server
> states (i.e., iOS) won't ever sync again until the server state is corrected

Even with some sort of batched write functionality in the iOS client, will it still be possible for them to be stranded by other not-updated-to-use-batches-yet clients that corrupt the server state?
Flags: needinfo?(rnewman)
Yes. But that's slightly less pernicious: those clients should eventually try again, and if they never do, there's little we can do. 

Eventually we can wire this in for other clients, too.
Flags: needinfo?(rnewman)
I'm cautiously optimistic about implementing something like this.  Lots of thoughts so I'm going to ramble a bit below.

---

I have many questions about details and edge-cases.  Some off the top of my head:

* Do you need to be able to include deletions in the actions applied as a batch, or only uploads of new or modified records?

* Do you need all items included in the batch to wind up with the same modified time when committed?  I suspect such a requirement would make clients behave better and the server behave worse.

* Do we need to support partial-update semantics on the records, e.g. bumping ttl without changing the payload?

* Can we impose an upper limit on the number of items in a batch, for db sanity sake?  Letting clients force us into copying millions of rows around as a single atomic operation would not be great for performance.

---

My gut says we're better off treating a "batch" as a separate thing rather than pretending that it's a special kind of collection, but I think we could do so in a way that keeps the good properties of the initial proposal.  Here's a counter-counter-proposal for an API that parallels the way we do batched downloads.

You start by uploading some items as usual, but requesting they form the start of a batch by using a new query parameter:

   POST /storage/bookmarks?batch=true HTTP/1.1
   X-If-Unmodified-Since: 12345
   [...send the first set of bsos in this request...]
  
   .
  
   HTTP/1.1 202 Accepted
   X-Last-Modified: 12345
   {
    "batch": "abcdef",
    "success": [...],
    "failed": {...}
   }

You *must* specify an X-If-Unmodified-Since on all requests involving a batch, and we won't accept the write if the collection has been modified.  If the items are accepted then you get back an opaque token that you can use to continue adding items to the batch:

   POST /storage/bookmarks?batch=abcdef HTTP/1.1
   X-If-Unmodified-Since: 12345
   [...more bsos to add...]
  
   .
  
   HTTP/1.1 202 Accepted
   X-Last-Modified: 12345
   {
    "batch": "abcdef",
    "success": [...],
    "failed": {...}
   }

When you're sending the last bunch of items, tell the server to commit the whole batch:

   POST /storage/bookmarks?batch=abcdef&commit=true HTTP/1.1
   X-If-Unmodified-Since: 12345
   [...any final bsos to add...]
  
   .
  
   HTTP/1.1 200 OK
   X-Last-Modified: 16789
   {
    "modified": "16789",
    "success": [...],
    "failed": {...}
   }

Only at this point does the modification time of the collection change, and all the new items become visible to other clients. 

I think this keeps many of the nice properties of Richard's proposal, in particular it makes the semantics of X-I-U-S very clear since all the calls are targeting a single resource.  But it avoids opening up any questions about how collection-like a "temporary collection" would be, such as whether you can read from it or delete from it.

It would also save two round-trips by including payloads in the "start transaction" and "commit transaction" requests.  You could make them separate steps by sending an empty body.

There's open questions around concurrent batched writes.  Would we allow multiple batches to be in-progress at once, erroring out only when one of the other manages to commit?  Or would we restrict each collection to only a single pending batch, and risk an abandoned batch upload blocking other writes until it times out?

---

Implementation-wise, I could see this working by using a separate possibly-sharded table to accumulate incoming batches of items.  We'd make the batch token encode a timestamp so that entries are added to the table in sorted primary-key order, and we'd store the metadata about a batch separately from the items so that we don't have to write the userid and collection on every row:

    CREATE TABLE batch_uploads (
      batch BIGINT NOT NULL PRIMARY KEY,
      userid INTEGER NOT NULL,
      collection INTEGER NOT NULL,
    )

    CREATE TABLE batch_upload_items (
      batch BIGINT NOT NULL,
      item VARCHAR(64) NOT NULL,
      modified BIGINT NOT NULL,
      sortindex INTEGER,
      payload TEXT,
      payload_size INTEGER,
      ttl INTEGER,
    ) PRIMARY KEY (batch, item)

Note there are on secondary indexes, so you couldn't e.g. look up whether there's an active batch upload on a particular user's collection.

Creating a batch just means inserting into the metadata table like we currently do for collections:

    INSERT INTO batch_uploads (batch, userid, collection)
    VALUES (:timestamp-based-batch-id, :userid, :collection)

As items are uploaded in batch mode, we write them into this separate table rather than the usual "bso" table.  Importantly, if a field is not set on the incoming bso, we leave it as NULL.  This allows us to properly apply partial-update semantics when we go to merge the batch into the collection.  We'd use an almost identical query to how we currently insert items into the main "bso" table:

    INSERT INTO batch_upload_items
      (batch, item, sortindex, modified, payload, payload_size, ttl)
    VALUES
      (...), (...), (...)
    ON DUPLICATE KEY UPDATE
      sortindex = VALUES(sortindex),
      payload = VALUES(payload)
      payload_size = VALUES(payload_size)
      ttl = VALUES(ttl)

For the commit step, we'd do a big old INSERT INTO ... SELECT to copy the data across, being careful to apply partial update semantics for any rows with NULL values:

    INSERT INTO bso
      (userid, collection, id, sortindex, modified, payload, payload_size, ttl)
    SELECT
      :userid, :collection, item, modified, sortindex,
      COALESCE(payload, ""), COALESCE(payload_size, 0),
      COALESCE(ttl, :default_ttl)
    FROM batch_upload_items
    WHERE batch = :batch
    ON DUPLICATE KEY UPDATE
      sortindex = COALESCE(VALUES(sortindex), sortindex),
      payload = COALESCE(VALUES(payload), payload),
      payload_size = COALESCE(VALUES(payload_size), payload_size)
      ttl = COALESCE(VALUES(ttl), ttl)

We'd have to experiment with the performance characteristics of this in practice.  We'd also delete the batch id, but probably not worry about deleting the actual batch items because IIRC random-access deletes can tend to be expensive:

    DELETE FROM batch_uploads WHERE batch = :batch

As a separate background task, we'd go through and delete all outstanding batches that are older than some threshold.  This cleans up items that have been applied, and items from uploads that have been abandoned:

    DELETE FROM batch_uploads WHERE batch < :some_threadhold_value

    DELETE FROM batch_uploadItems WHERE batch < :some_threadhold_value

This should be pretty cheap because it would be pruning an entire left subtree of the primary key.

---

With respect to timing:

> is there anything we can get done and deployed within the next four weeks?
> E.g., a single operation to insert into a collection by reading records from another,
> with none of the other neat server support?

I think the "copy items from somewhere else" part is the trickiest part of all this, at least in terms of operational unknowns.  So if we're serious about taking this approach, I'd prefer we try to ship a whole version of it, rather than e.g. having clients create their own "temporary" collections that show up in /info/collections, hide from operational metcs, etc.

If we can we dedicate a python person to this for a couple of weeks, dev work for what I've sketched above seems feasible to me.  Ops and QA, I don't know about.

---

Supposing we do go this route, it's interested to consider how we might measure success or failure.

I believe we have an open bug re: measuring instances of bookmarks tree corruption.  It would be very nice to be able to watch that metric either go down, or at least not increase when iOS rolls out.

I also wonder whether we can add some statsd counters to track the overall number of bookmarks records travelling into and our of storage over time.  Watching for changes in "number of bookmarks uploaded" or "number of bookmarks downloaded" or perhaps in the ratio between the two, could tell us whether messing with how bookmarks are uploaded has broken anything.

---

OK I'll stop now :-)
Summary: [meta] Atomic write support → [meta] Support atomic write of large batches of items
(and at this point, I'm missing github's ability to go back and stealth-edit all the typos in a big wall of text...)
(In reply to Ryan Kelly [:rfkelly] from comment #9)

> * Do you need to be able to include deletions in the actions applied as a
> batch, or only uploads of new or modified records?

From the client's perspective, 'deletions' are really just records with deleted=true, so no. We don't need to interleave removals of records from the server.


> * Do you need all items included in the batch to wind up with the same
> modified time when committed?  I suspect such a requirement would make
> clients behave better and the server behave worse.

Not strictly. Right now we already chain uploads and manually track all of the timestamps for each successful batch, so we have the client infrastructure in place to do so in this case. Semantically it makes more sense for everything to get the same timestamp, of course, and it's simpler for clients if that's the case.


> * Do we need to support partial-update semantics on the records, e.g.
> bumping ttl without changing the payload?

No.


> * Can we impose an upper limit on the number of items in a batch, for db
> sanity sake?  Letting clients force us into copying millions of rows around
> as a single atomic operation would not be great for performance.

I expect so. So long as we can get up to, say, 50,000 (the most bookmarks I've seen someone have) we're probably OK -- more than that and we're heading towards exhausted quotas anyway.


…

> It would also save two round-trips by including payloads in the "start
> transaction" and "commit transaction" requests.  You could make them
> separate steps by sending an empty body.

Yeah, that helps.

The mutable batch token is slightly inconvenient for a functional-programming-style Swift client -- the benefit that includes payloads in start and end requests complicates our state approach -- but we can make it work.


> There's open questions around concurrent batched writes.  Would we allow
> multiple batches to be in-progress at once, erroring out only when one of
> the other manages to commit? 

I see no reason why not. After all, one could be abandoned by a client with a flaky network connection, and it's cruel to exclude other clients until… when, two hours later?

> Or would we restrict each collection to only a
> single pending batch, and risk an abandoned batch upload blocking other
> writes until it times out?

Quite.


> If we can we dedicate a python person to this for a couple of weeks, dev
> work for what I've sketched above seems feasible to me.  Ops and QA, I don't
> know about.

Roger that.


> I believe we have an open bug re: measuring instances of bookmarks tree
> corruption.  It would be very nice to be able to watch that metric either go
> down, or at least not increase when iOS rolls out.

Yeah.

I don't expect iOS to increase this rate, particularly compared to desktop (which almost goes to extra lengths to cause damage!) and Android (which should be just as network-unreliable as iOS, and is much slower).

One day iOS will be able to send us telemetry assessing the validity of server bookmark data…

Amusingly, I don't believe desktop is able to measure validity, precisely because it never has a consistent snapshot of the server data. Only iOS does.


> OK I'll stop now :-)

You're the best :D
> The mutable batch token

My intention was that the batch token be opaque, but not change for the lifetime of the batch (unlike the offset token which changes with each chunk you've downloaded).
:bobm, the batch-related queries I outlined in Comment 9, do they scare you? :-)
Flags: needinfo?(bobm)
Blocks: 1252469
(In reply to Ryan Kelly [:rfkelly] from comment #13)
> :bobm, the batch-related queries I outlined in Comment 9, do they scare you?
> :-)

Not necessarily.  Modifying the behavior of presently running nodes to work in this fashion has some complications.  It would be great if we could roll out new nodes and maintain backward compatibility with the present nodes.

The batch_upload_items table might be a good candidate for a clustering secondary key: https://www.percona.com/blog/2009/06/04/how_clustering_indexes_sometimes_help_update_and_delete_performance/

Also, I suspect it will need frequent optimization.

This also has implications for general POST times, and the timeouts associated with them.  Are we considering sharding across multiple batch_upload_items or using a single table?  In the former case, there are potential space and memory problems, so we may need to do some re-sizing such as users per node, instance size, etc.  In the latter, populating new users into a node after a migration gets tricky.
Flags: needinfo?(bobm)
> This also has implications for general POST times, and the timeouts associated with them.

It's not obvious to me what those implications are, can you please say more about this concern?

> Are we considering sharding across multiple batch_upload_items or using a single table?

I think it would make sense to shard this table in the same way we shard the bso table.

> In the latter, populating new users into a node after a migration gets tricky.

Why?
Flags: needinfo?(bobm)
> > * Do you need all items included in the batch to wind up with the same
> > modified time when committed?  I suspect such a requirement would make
> > clients behave better and the server behave worse.
> 
> Not strictly. Right now we already chain uploads and manually track all of the timestamps for each
> successful batch, so we have the client infrastructure in place to do so in this case. Semantically it 
> makes more sense for everything to get the same timestamp, of course, and it's simpler for clients if 
> that's the case.

What worries me here is other clients missing items from a batch, if they update their notion of "I have all changes before timestamp X" using the current time rather than the last-modified time of the collection.  Consider a flow like:

  * Client A starts uploading a batch at T1
  * Client B syncs at T2, sees no changes, updates its high-water-mark timestamp to T2
  * Client A adds more items to the batch at T3
  * Client A commits the batch at T4
  * Client B syncs at T5, notices there are changes, requests /storage/bookmarks?newer=T2

If the batch contains items with timestamp T1, then client B will not retrieve them.  Given all batch items the timestamp of the commit would avoid this problem and is probably the right thing to do, despite possible negative effects on the server.
Here's a quick attempt at turning my proposed API into a concrete docs update.  It's surprisingly small because I think many of the semantics around e.g. x-if-unmodified-since are implied by default from their semantics on an ordinary POST request.  Richard, Danny, thoughts?

One interesting thing about the API here - apart from managing the "batch" and "commit" query parameters, the series of requests it has to make is exactly the same whether it's doing a new batch-based upload or a more-prone-to-corruption series of individual POSTs.  If wonder if that opens up any possibilities for building conditional support into the client, allowing it to take advantage of batching if/when the server offers it but falling back to individual POSTs when it doesn't (or if it's uploading more items than the server is comfortable dealing with in a single batch).
Attachment #8725528 - Flags: feedback?(rnewman)
Attachment #8725528 - Flags: feedback?(dcoates)
(In reply to Ryan Kelly [:rfkelly] from comment #17)
> If wonder if that
> opens up any possibilities for building conditional support into the client,
> allowing it to take advantage of batching if/when the server offers it but
> falling back to individual POSTs when it doesn't (or if it's uploading more
> items than the server is comfortable dealing with in a single batch).

Both android and (now) desktop are already setup with a "batch" abstraction to try and sanely deal with the 1MB limit, so that seems reasonable. Relatedly, it seems this batch mode would be a subtle change in semantics - if one of the POSTs generated a 413, previously stuff POSTed before would be written, whereas now I assume they would not (that the batch would fail).

I think that's OK (the batch *should* fail) but it's worth calling out.
(In reply to Ryan Kelly [:rfkelly] from comment #17)

> One interesting thing about the API here - apart from managing the "batch"
> and "commit" query parameters, the series of requests it has to make is
> exactly the same whether it's doing a new batch-based upload or a
> more-prone-to-corruption series of individual POSTs.

Yeah, that's one of the reasons I suggested this kind of approach (mine 'wrapped', yours 'annotated') -- it's still just POSTing records, so not too many changes to risky client code.


> … falling back to individual POSTs when it doesn't (or if it's uploading more
> items than the server is comfortable dealing with in a single batch).

Yeah, probably. As Mark notes, the main client change would be in success/failure tracking: with individual batches the client needs to do bookkeeping after each response.
Comment on attachment 8725528 [details] [diff] [review]
docs PR adding proposed batch functionality

This looks good. Comments on the PR.

In particular, I'd like to see a little more clarity about where success/failure are reported/detected.

Sidenote: is there a list of failure 'keys' anywhere? It just struck me that some will be transient and some permanent, and the client cares!
Attachment #8725528 - Flags: feedback?(rnewman) → feedback+
> is there a list of failure 'keys' anywhere? 

No, but we should add one.
See Also: → 1252997
> No, but we should add one.

Filed Bug 1252997.
> if one of the POSTs generated a 413, previously stuff POSTed before would be written,
> whereas now I assume they would not (that the batch would fail)

Indeed, the intention would be that the whole batch fails, need to clean up the docs to make that clearer.
(In reply to Ryan Kelly [:rfkelly] from comment #15)
> > This also has implications for general POST times, and the timeouts associated with them.
> 
> It's not obvious to me what those implications are, can you please say more
> about this concern?

Having gone back over the proposal, outside the actual commit the individual record POSTs probably won't deviate much from the present.  However, I would expect the commit query to take up the number two spot in our slow query logs.  Which means we move the outlier long running queries bar to slightly longer queries.

Perhaps I'm way wrong about this, but it seems we could be opening clients up to a scenario of eventually finding the right batch combination that never completes, and trying it forever.  My fear of this isn't great, but it would be great to generate batch specific stats: success vs. failure, average number of items per batch, average size of batch, average time to commit from batch table to bso table, plus similar stats around batch table cleanup- and then load test it till it falls over.  

> > Are we considering sharding across multiple batch_upload_items or using a single table?
> 
> I think it would make sense to shard this table in the same way we shard the
> bso table.

Sounds good.  It's probably time to re-size instances anyway.

> > In the latter, populating new users into a node after a migration gets tricky.
> 
> Why?

Not sharding would mean lots of users making full uploads to the same batch table at the same time on a node migration.  Maybe it wouldn't be a problem, given the constant pruning.  But we're talking about sharding anyway.  There's would be lot of load testing to done to tune this properly.
Flags: needinfo?(bobm)
>  it would be great to generate batch specific stats

Huge +1 to this.
>  we could be opening clients up to a scenario of eventually finding the
> right batch combination that never completes, and trying it forever.

Right, a bad failure case here would be clients trying to upload a batch that's bigger than we're willing to accept, failing out and retrying the whole batch over and over again.  We'd need a clear way to communicate "stop adding items to this batch dammit, it's full" and have clients respect that.
Blocks: 1253051
(In reply to Ryan Kelly [:rfkelly] from comment #26)
> >  we could be opening clients up to a scenario of eventually finding the
> > right batch combination that never completes, and trying it forever.
> 
> Right, a bad failure case here would be clients trying to upload a batch
> that's bigger than we're willing to accept, failing out and retrying the
> whole batch over and over again.  We'd need a clear way to communicate "stop
> adding items to this batch dammit, it's full" and have clients respect that.

We already have that scenario now - when a single record results in a 413, the client is going to effectively be stuck and continually retry that record. This new scenario isn't that different, other than it will be the entire batch rather than just part of it. If the client tries to mitigate this and only uploads part of the batch, ISTM we are then in the exact same scenario we are trying to avoid (ie, having the server state be inconsistent). When thinking about the 413 case I almost suggested the batch could auto-commit before returning a 413 for the records that's too large (ie, committing the parts of the batch that were already accepted), but that would have the same problem.

(This is under the assumption that "single record exceeds some post max size" is the same basic problem as "all records exceed some batch max size", but I might be missing some subtlety here)
(In reply to Mark Hammond [:markh] from comment #27)
> (This is under the assumption that "single record exceeds some post max
> size" is the same basic problem as "all records exceed some batch max size",
> but I might be missing some subtlety here)

I guess one obvious subtlety is that the largest batch uploaded by a client is quite likely to be the very first sync of a device - so there's a risk we wouldn't get a single bookmark uploaded :/
(In reply to Mark Hammond [:markh] from comment #28)

> I guess one obvious subtlety is that the largest batch uploaded by a client
> is quite likely to be the very first sync of a device - so there's a risk we
> wouldn't get a single bookmark uploaded :/

To be clear: that's a *good* thing. We never want anything less than all of a user's bookmarks, because anything less almost certainly results in corruption on every other client. And if there's no other client, then we're in no rush to upload.

We don't have a good answer for when one bookmark -- typically the Unsorted Bookmarks folder -- is too big to upload. Probably the only thing we can do is introduce N "Unsorted Bookmarks N" folders and split up the user's bookmarks automatically. This would be straightforward to do on iOS.
(In reply to Ryan Kelly [:rfkelly] from comment #26)

> Right, a bad failure case here would be clients trying to upload a batch
> that's bigger than we're willing to accept, failing out and retrying the
> whole batch over and over again.  We'd need a clear way to communicate "stop
> adding items to this batch dammit, it's full" and have clients respect that.

We should 4xx in a recognizable way immediately when that case is detected. If a user's bookmarks can't fit into a single batch, they cannot safely sync bookmarks at all if there are other clients.

This situation is also why the batch limit should be on the order of ~40-50,000 -- we have users with that many bookmarks, and those are the users who right now are issuing 500 HTTP POSTs and relying on them all succeeding to avoid corruption.

(And that's motivation to bump the number in Bug 1250747 -- it reduces the number of requests a client needs to make to something less crazy.)


(In reply to Bob Micheletto [:bobm] from comment #24)

> Perhaps I'm way wrong about this, but it seems we could be opening clients
> up to a scenario of eventually finding the right batch combination that
> never completes, and trying it forever.

That will be the case if they hit a server limit of some kind -- e.g., a batch where committing exceeds some query timeout. We should make sure that doesn't routinely happen :D

If it helps, the client doesn't need to synchronously wait around for the results, so long as the next visitor will block until the commit succeeds. We don't care about anything other than the list of successful IDs and the timestamp they'll commit with.


> … commit from batch table to bso table, plus similar stats around batch table
> cleanup- and then load test it till it falls over.  

+1.


> Not sharding would mean lots of users making full uploads to the same batch
> table at the same time on a node migration.  Maybe it wouldn't be a problem,
> given the constant pruning.  But we're talking about sharding anyway. 
> There's would be lot of load testing to done to tune this properly.

We expect most users -- as now -- to have small bookmark libraries. Anyone under 100 bookmarks doesn't even need this API, and so will issue one-shot requests. Atomic writes/batching/committing is really a replacement for a stream of lots and lots of upload requests.
> I guess one obvious subtlety is that the largest batch uploaded by a client
> is quite likely to be the very first sync of a device - so there's a risk we
> wouldn't get a single bookmark uploaded :/

Oh, and in the general case:

I expect transactional upload like this to help a lot with first syncs. Clients can take as long as they like (within reason) to upload to their private staging zone before committing, and will only fail there if another client races. Long-lived batch uploading makes clients robust against intermittent failures, and saves them from redownloading the world and reuploading the world over and over.
> We should 4xx in a recognizable way immediately when that case is detected.
> If a user's bookmarks can't fit into a single batch, they cannot safely
> sync bookmarks at all if there are other clients.

In which case, we should require you to send the intended batch size up-front when starting a new batch.  This would significantly reduce the potential for client retrys to impact overall server load.  Is that feasible from the client's POV?
Flags: needinfo?(rnewman)
> This situation is also why the batch limit should be on the order of ~40-50,000

Relatedly, it would be neat if clients could discover the actual size limits enforced by the server without having to either guess, or resort to trial and error...
(In reply to Ryan Kelly [:rfkelly] from comment #32)
> In which case, we should require you to send the intended batch size
> up-front when starting a new batch.  This would significantly reduce the
> potential for client retrys to impact overall server load.  Is that feasible
> from the client's POV?

Assuming you are talking the number of records, this is known by desktop. What isn't known up-front is the total number of bytes (and thus the number of necessary POSTs also isn't known)
I'll settle for total number of records.
Blocks: 1253111
Blocks: 1253112
(In reply to Ryan Kelly [:rfkelly] from comment #32)

> In which case, we should require you to send the intended batch size
> up-front when starting a new batch.  This would significantly reduce the
> potential for client retrys to impact overall server load.  Is that feasible
> from the client's POV?

Yes, very much so!

iOS knows both record count and byte count. Android might know both. Desktop only knows record count.

I would be tempted to phrase this as "I'm going to send at least this many records; should I even bother?", rather than a hard contract -- some clients might under-count a little, but it's better than nothing.

> Relatedly, it would be neat if clients could discover the actual size limits
> enforced by the server without having to either guess, or resort to trial
> and error...

Yeah, I was going to discuss that: I'm pretty sure our clients don't behave correctly if the existing per-batch limit is reduced from the default config.

They don't really negotiate with the server, even though the server returns failed IDs, and of course they would have to fail at least once to do so.

It'd be great if they had a way to figure those things out.

Perhaps a header in a collection GET response, immediately before the upload, or as metadata on the i/c response?

  X-Upload-Parameters: max: 50000, batchN: 200, batchK: 1000, recordK: 256

Have to pick some way of doing this that allows operational tuning…
Flags: needinfo?(rnewman)
I wouldn't expect the limits to change often, so sending it on every GET sounds like a lot of wasted bytes on the wire.   Perhaps we can just make a new "GET /info/configuration" endpoint that will tell you any server-specific configuration bits that could affect how you use the protocol, including:

* max items per POST request
* max bytes per POST request
* max bytes per request bode
* max items per batch
* max bytes per batch

(This would also be useful for our functional tests actually, some of which cant be run against a live server precisely because they depend on specific bits of config)
WFM!
(In reply to Ryan Kelly [:rfkelly] from comment #37)
> I wouldn't expect the limits to change often, so sending it on every GET
> sounds like a lot of wasted bytes on the wire.   Perhaps we can just make a
> new "GET /info/configuration" endpoint that will tell you any
> server-specific configuration bits that could affect how you use the
> protocol, including:
> 
> * max items per POST request
> * max bytes per POST request
> * max bytes per request bode
> * max items per batch
> * max bytes per batch

The only downside I see of that is that if the client chooses to not even make the request due to the batch size being exceeded, we've potentially lost the ability for server-side metrics to tell us what limit would have caused success.

An alternative may be that the first POST that starts the batch includes what the client knows (eg, total number of records, number of bytes if known) and for the server to fail that first POST if the limit is exceeded. That doesn't help with the "max bytes per POST" though (the client almost certain *can* adapt to that in the common case)
> An alternative may be that the first POST that starts the batch includes what the client knows
> (eg, total number of records, number of bytes if known) and for the server to fail that first
> POST if the limit is exceeded.

This is basically what I want to run with - the client sends an X-Weave-Batch-Records and/or X-Weave-Batch-Bytes header when it knows these values up-front, and the server gives you a 400 error with recognizable error code when those values would not be accepted.  Updated API docs incoming.
Comment on attachment 8725528 [details] [diff] [review]
docs PR adding proposed batch functionality

I've adjusted the API proposal based on the above discussion, see linked PR.
Attachment #8725528 - Flags: feedback?(rnewman)
Attachment #8725528 - Flags: feedback?(markh)
Attachment #8725528 - Flags: feedback+
Comment on attachment 8725528 [details] [diff] [review]
docs PR adding proposed batch functionality

LGTM!
Attachment #8725528 - Flags: feedback?(markh) → feedback+
Blessings upon all your houses for writing the docs first.  It's a real pleasure to watch this come together in a testable fashion.
> is there anything we can get done and deployed within the next four weeks?

Richard, can you please give us precise dates/deadlines relevant to this from the iOS perspective?  I'm working on resources and a plan from our side.
Flags: needinfo?(rnewman)
Deadlines are in short supply :)

In short: if you can get it out the door this week or next, great. If not, take as long as you need within reason.


Planned submission date for 3.0 is March 22, so in order to go live in 3.0 the server feature would need to be built and deployed by March 17/18 or so… and even that is a little risky.

(Calendar: <https://wiki.mozilla.org/Firefox_for_iOS_Train_Schedule>)

If it doesn't make that, it doesn't make it; my inability to propose this work sooner doesn't make this your firedrill!


We will need to make a call within the next two weeks about whether we will ship bookmark sync in 3.0, given two things:

* The unknown state of bookmark trees in the wild. I'm adding telemetry now to try to figure that out in prerelease.
* How likely we are to break things ourselves: this bug plus Bug 1253112.

(Without atomic writes, an interrupted upload from the iOS device will result in it very likely refusing to sync again without manual intervention. That doesn't seem like a good risk to take.)


If Bug 1250747 is fully deployed to production by then (or even if not!) there is a middle ground: we can sync chunks up to the limit, and not sync at all beyond that.
Flags: needinfo?(rnewman)
> If it doesn't make that, it doesn't make it;
> my inability to propose this work sooner doesn't make this your firedrill!

Acked and understood :-)

Realistically we can't ship a fully QA'd version of this to the server that timeframe.  But I'm wonder whether we can agree on a forwards-compatible API that would allow you to bake in the necessary extra bits in the iOS client, while gracefully falling back to non-atomic uploads  on old versions of the server.

For example, providing a combination of:

* the proposed /configuration endpoint for a little bit of discoverability
* a dev server with the batching API implemented for testing purposes

Might allow you to ship client code that introspects the server's response and either weaves through `batch=foo` to do atomic writes, or omits it and hopes for the best, depending on whats supported by the server.
In particular, adding the configuration endpoint will also let us QA our way up to Bug 1250747 in a careful fashion.
(In reply to Richard Newman [:rnewman] from comment #45)
> (Without atomic writes, an interrupted upload from the iOS device will
> result in it very likely refusing to sync again without manual intervention.
> That doesn't seem like a good risk to take.)

To be clear, is it true that an interrupted upload on *any* device is just as likely to put iOS into this state?
Late to the conversation but I've only just gotten involved.

* Two hours?  That seems generous to a fault.  Is a client really likely to *not* simply restart a transaction if the one it's been working on has taken more than 10 minutes?

* .../info/configuration seems broadly named to me for the specific type of info contained therein.  Would .../info/limits be more accurate or is this intended to be the beginning of more client discoverable info?
Flags: needinfo?(rnewman)
Flags: needinfo?(rfkelly)
> * .../info/configuration seems broadly named to me for the specific type of info contained therein.  

I don't feel strongly about naming here, but if we ever did want to more client-discoverable info, it would feel pretty weird to have to make a parallel "/info/timeouts" or similar endpoint.
Flags: needinfo?(rfkelly)
(In reply to Ryan Tilder [:rtilder] from comment #49)

> * Two hours?  That seems generous to a fault.  Is a client really likely to
> *not* simply restart a transaction if the one it's been working on has taken
> more than 10 minutes?

If a client can't upload its batch within this time limit, it'll _never_ successfully do so, so we should avoid being cruel.

Guesstimate how long it'd take to do twenty or fifty thousand bookmarks from an Atom netbook on dial-up (or a Gingerbread phone on 2G), and let's go with that?

Last I checked, _downloading_ and applying 5000 records on a shitty phone takes about five minutes, so uploading (much slower) for our 99th-percentile users could well take an hour. But that's napkin math.



(In reply to Mark Hammond [:markh] from comment #48)

> To be clear, is it true that an interrupted upload on *any* device is just
> as likely to put iOS into this state?

If the interruption is permanent, yes: that's a generic partial write resulting in inconsistency.

If the interruption is transient, and the other client eventually uploads a consistent set of records, iOS will cheerfully sync when that state is downloaded

iOS is special in that it will refuse to sync if it sees inconsistency, so it ironically can't self-recover from its own partial writes.

We could add code to do so in most cases -- essentially allowing uploads to proceed across restarts -- but the same problem is solved more robustly by atomic upload work.



(In reply to Ryan Kelly [:rfkelly] from comment #46)

> Realistically we can't ship a fully QA'd version of this to the server that
> timeframe.  But I'm wonder whether we can agree on a forwards-compatible API
> that would allow you to bake in the necessary extra bits in the iOS client,
> while gracefully falling back to non-atomic uploads  on old versions of the
> server.

And we need to make that work anyway for eventual third-party server support (and, I suppose, China servers?).
Flags: needinfo?(rnewman)
For additional safety, we could also consider limiting the number of uncommitted batches per userid.
> * Two hours?  That seems generous to a fault.

Oh, but: time _since last write_, sure.


> For additional safety, we could also consider limiting the number of
> uncommitted batches per userid.

+1
Attachment #8725528 - Flags: feedback?(rnewman) → feedback+
Attachment #8725528 - Attachment is patch: true
No longer blocks: 1196238, 1252469
Assignee: nobody → rtilder
Status: NEW → ASSIGNED
Blocks: 1257961
:rnewman gave me some more information on what the partial write problem is since it wasn't clear in his initial description. Basically some BSOs reference other BSOs thus the need for an atomic all or nothing change to multiple BSOs. Since we have a number and size limit sometimes it's not possible to do a big update in a single POST request. 

Anyways, I had a random #showerthought about storing these payloads. Building off of :rfkelly's epic comment #9, I would suggest we make the batch table like this: 

    CREATE TABLE batch_uploads (
      batch unsigned BIGINT NOT NULL,
      userid INTEGER NOT NULL,
      collection INTEGER NOT NULL,
      body text NOT NULL,
      contentType enum("json", "newline") NOT NULL, 
      created datetime,
      primary key(userid, batch)
    )

Drop the batch_upload_items table. When a batch needs to be committed the body data is concatenated together and treated like a extra large POST. Don't write real records until we have to. 

This should have some advantages:

1. parse/verify later
2. fewer records and less indexing requirements
3. deletes should be easier as there are fewer rows
4. optimizations on tables I would think would remain the same
5. easier to clean up when an X-I-U-S precondition fails

On the API side: 

Start with: 

   POST /storage/bookmarks?batch=12345 HTTP/1.1
   X-If-Unmodified-Since: 12345.00
   [...more bsos to add...]

Finalize with:

   POST /storage/bookmarks?batch=12345&commit=true HTTP/1.1
   X-If-Unmodified-Since: 12345.00
   [...more bsos to add...]

This would simplify the batch parameter and make it a single type. Though, clients will have to choose a fairly large batch id themselves. It's a 64bit integer so pretty unlikely to be collisions. I'm 1/2 baked on making it client specified. Though taking the first 8 bytes of a SHA1(body) would give a pretty batch id. :)
One downside of not parsing records up-front is that we lose validation (Comment 30) -- the client might upload 50,000 records, only to find at the very end that one of them is too big or malformed.

This is the worst possible situation from a bandwidth/retry approach: the current codebase will simply retry all 50,000 records again on every sync, rather than failing the upload at the malformed record.

But if it buys significant simplicity on the server side, then that might be worthwhile.
(In reply to Richard Newman [:rnewman] from comment #55)
> One downside of not parsing records up-front is that we lose validation
> (Comment 30) -- the client might upload 50,000 records, only to find at the
> very end that one of them is too big or malformed.

Good point. Validating the body at POST and responding appropriately would be better. We would probably wind up validating again at commit time. That's OK, since we're good on CPU/RAM, but tightly constrained on IO.
No longer blocks: 1253051
Depends on: 1253051
Couple of implementation questions.  Apologies for the random ordering, I've been collecting them over the last week and a bit.

How should we shard the batch_upload_items tables?  Right now it's using the same method as the bso tables because it was low hanging fruit.  The only complication there is that userid isn't common to both the batch_upload and batch_upload_items tables.  The batch ID is, however.  I don't particularly care either way but I suspect Bob might in the end.  At the moment, I suspect staying with the current methodology leads to (hopefully) more predictable performance profiles.


The bso table has an 'id' column while the batch_upload_items has an 'item' column.  I don't know if you would prefer to go for consistency.  I don't care either way.


Should we drop use of the word "batch" in the case of the transaction tables since it could lead to confusion with batch uploads?  Related but not the same.


What is the expected behaviour if a client tries to upload to a completed or aborted transaction?  Error out(if so, with what?) or magically instantiate a new transaction and hope the client realizes that.
(In reply to Ryan Tilder [:rtilder] from comment #57)

> What is the expected behaviour if a client tries to upload to a completed or
> aborted transaction?  Error out(if so, with what?) or magically instantiate
> a new transaction and hope the client realizes that.

If you've already returned OK or failure to the client for this transaction: 400 bad request.

If this is a server error (that is: something failed, but the client hasn't been told): 5xx.
> How should we shard the batch_upload_items tables?
> Right now it's using the same method as the bso tables because it was low hanging fruit.

This seems reasonable to me.

> The bso table has an 'id' column while the batch_upload_items has an 'item' column.
> I don't know if you would prefer to go for consistency.

Consistency++, I support making them be named the same.

> Should we drop use of the word "batch" in the case of the transaction tables
> since it could lead to confusion with batch uploads?  Related but not the same.

This sounds similar to unresolved naming questions in [1], and I agree we should rename things as appropriate to avoid confusion.  Something about calling these things "transactions" rubs me the wrong way, as they're a lot more limited than a traditional database transaction.  But I don't have a concrete alternative to suggest.

> What is the expected behaviour if a client tries to upload to a completed or aborted transaction?

+1 to :rnewman's comment, specifically I see the following ways it could go wrong:

* Upload with an unknown trn id  ->  400 bad request
* Upload to an already committed trn id  ->  400 bad request
* Something went horribly wrong but it's not obvious what  ->  500 server error

I don't know what it would mean for the client to "upload to an aborted transaction" and how that would differ from e.g. a transaction that never existed in the first place.

[1] https://github.com/mozilla-services/docs/pull/60
> As a separate background task, we'd go through and delete all outstanding batches that are
> older than some threshold.  This cleans up items that have been applied, and items from uploads
> that have been abandoned:
>
>    DELETE FROM batch_uploads WHERE batch < :some_threshold_value
>
>    DELETE FROM batch_upload_items WHERE batch < :some_threshold_value
>
> This should be pretty cheap because it would be pruning an entire left subtree of the primary key.

Of all of the implementation details of this bug, this conjecture scares me the most.  I think it should be OK, but maybe we can cheaply try it out in a production-like environment?
(In reply to Ryan Kelly [:rfkelly] from comment #59)
> > Should we drop use of the word "batch" in the case of the transaction tables
> > since it could lead to confusion with batch uploads?  Related but not the same.
> 
> This sounds similar to unresolved naming questions in [1], and I agree we
> should rename things as appropriate to avoid confusion.  Something about
> calling these things "transactions" rubs me the wrong way, as they're a lot
> more limited than a traditional database transaction.  But I don't have a
> concrete alternative to suggest.
> 

Currently a transaction is a group of batches.  The obvious plural of batches in this case is murder: a murder of batches.
Maybe we need the words 'part' and 'whole' here?
> Currently a transaction is a group of batches.

This is exactly the problem we're having - when I initially wrote the API I had "a batch is a group of individual POSTs" in my head, but that obviously doesn't mesh with what everyone thinks when they hear the word "batch".  I hate to bikeshed, but the current naming seems to active encourage confusion, we should try to come up with something better.  Strawman:

  POST /storage/bookmarks?partial=true

  POST /storage/bookmarks?partial=true&changeid=123456

  POST /storage/bookmarks?partial=true&changeid=123456&commit=true
Depends on: 1269902
Blocks: 1269902
No longer depends on: 1269902
No longer blocks: 1257961
Blocks: 1269904
No longer blocks: 1269902
Depends on: 1271466
Attachment #8725528 - Flags: feedback?(dcoates)
Depends on: 1273100
Depends on: 1273102
No longer depends on: 1271466
Depends on: 1250747
Depends on: 1223226
Whiteboard: [data-integrity]
(I originally added this comment to the wrong bug)

Any chance of getting guidance (ideally in the docs added in https://github.com/mozilla-services/docs/pull/60/files, but here would be OK too) for when the new /info/configuration endpoint should be called (ie, how long, if at all, the client should cache it for)?  I'm *guessing* it's reasonable to cache in memory for the session (but still after a node reassignment) etc, or should I call it once per-sync?
/info/configuration is meant to be a query-at-HTTP-session-start based on my understanding of the original spec written by rfkelly.  I don't see why it wouldn't be feasible to cache it for up to 4 hours or so, though.  Changes are going probably going to be *very* infrequent.
A few questions as I start an implementation:

* The server is returning {"max_post_records":100,"max_request_bytes":1048576,"max_total_bytes":104857600,"max_total_records":10000,"max_post_bytes":1048576}, but the pull-request suggests I should be expecting |max_batch_records| and |max_batch_bytes|. I assume the |_total_| fields returned are serving this purpose, but is the PR for the docs now out of date, or is the server sending the wrong field names?

* Currently the client will still attempt to POST a single WBO that's greater than the max-size "just incase" it happens to work. The docs don't seem clear on the batch semantics in this case - will a 4XX response as part of a batch kill the entire batch? To put it another way, am I free to attempt to exceed the limits, handle the 4XX and continue as normal, or will this kill the entire batch?

* The docs suggest |Where possible, clients should use the *X-Weave-Batch-Records** and *X-Weave-Batch-Bytes* headers to signal the expected batch size, so that oversize batches can be rejected before the items are uploaded.| I'm wondering why this is necessary given we've already been told what the max sizes are by the server, and thus the client will ensure it never exceeds this limit, creating multiple batches if necessary? (FWIW, the client doesn't know the numBytes up-front - it probably can specify the upper-limit of records though, but given the above it doesn't really seem worthwhile.

* There will be cases where we will make a final POST with commit=true but with no records - when we made the last POST we weren't aware that this was the last record. I'd like to confirm this isn't a problem in practice (and it might even be worth adding this to the docs?)

Probably more to come later...
(In reply to Mark Hammond [:markh] from comment #67)
> A few questions as I start an implementation:

A few answers as the impl takes shape :)

> but is the PR for the docs now out of date, or is the server
> sending the wrong field names?

I'd still like to know that. ni? rtilder for that.

> will a 4XX response as part of a batch kill the entire batch?

If the server doesn't, Sync will - it will just stop on a 4XX anyway.

> * The docs suggest |Where possible, clients should use the
> *X-Weave-Batch-Records** and *X-Weave-Batch-Bytes* headers to signal the
> expected batch size

So yeah, I'm not doing any of that now.

> * There will be cases where we will make a final POST with commit=true but
> with no records

That turns out to be "impossible" for desktop. Also, the docs do explicitly say this is OK.

> Probably more to come later...

One minor point is that the docs refer to x-last-modified, but the desktop implementation is actually using x-weave-timestamp. While Ryan assures me the 2 will be the same for write operations, it is a little smelly that the code looking at the batch semantics uses one header but Sync's internal timestamp tracker uses a different one. This is more a comment than a question (but thoughts welcome;)

Final question: should we have a way to explicitly "roll back" (ie, tell the server to discard) a batch? Let's say Sync discovers a single item larger than allowed in a POST. It might decide to abort at that point, but the server doesn't know leaving stuff behind for the "garbage collector". I've no insights into how often we might expect that occur in practice though.
Flags: needinfo?(rtilder)
> but is the PR for the docs now out of date, or is the server
> sending the wrong field names?

As an implementer interpreting these docs, which one do you think makes more sense?  In Bug 1273100 there's mention that the noun "batch" was confusing, it wasn't entirely clear whether it referred to a part or the whole.  I think this was what motivated the server sending "Total-Bytes" instead.

> One minor point is that the docs refer to x-last-modified,
> but the desktop implementation is actually using x-weave-timestamp.

I suspect that the protocol spec guarantees these will be the same, if we go through it with a fine-enough-toothed comb.

> should we have a way to explicitly "roll back" (ie, tell the server to discard) a batch?

My guts sense is this won't be worth the extra complexity, and we should just let it be dealt with via normal GC.  :rtilder, thoughts?
Flags: needinfo?(markh)
(In reply to Ryan Kelly [:rfkelly] from comment #69)
> As an implementer interpreting these docs, which one do you think makes more
> sense?  In Bug 1273100 there's mention that the noun "batch" was confusing,
> it wasn't entirely clear whether it referred to a part or the whole.  I
> think this was what motivated the server sending "Total-Bytes" instead.

TBH, I don't really care what it is - I think we should just merge those docs (with whatever changes are needed to match the impl) so we have canonical docs.

(From Desktop's internal POV, "batch" makes more sense, as it may split its "total" across many batches. I can see why the server considers it a "total" though)
Flags: needinfo?(markh)
Whiteboard: [data-integrity] → [sync-atomic]
> I think we should just merge those docs (with whatever changes are needed to match the impl)
> so we have canonical docs

Done, the docs are merged and should match the current implementation.  If you find any places where they differ, please point it out.
Noting that we need to make sure to track the max body size configuration when we push this out: See https://bugzilla.mozilla.org/show_bug.cgi?id=1293496#c2
I noticed that the responses we get back while POSTing within a batch don't contain the 'modified' property but is included on the commit POST. :tcsc mentioned that this is ignored on Desktop where the X-Last-Modified property is used. On iOS, we make use of it being in the JSON response. Was there any reason for not including it in the JSON on the POSTs during a batch and is there a chance we could add it back in?
needinfo to rfkelly, 'cos I think rtilder is on vacation.
Flags: needinfo?(rfkelly)
Keywords: meta
> I noticed that the responses we get back while POSTing within a batch don't contain 
> the 'modified' property but is included on the commit POST.

In an attempt to keep sync's fragile notion of time as consistent as possible, we decided that all items posted during a batch would receive the same last-modified time, and that last-modified time would be the timestamp at which the whole batch is committed.  So we can't actually report a meaningful timestamp until the commit request, because we don't know what it will be yet.

If that turns out to be a PITA in practice, we can try to figure out another approach, but that's the thinking behind the current approach.
Flags: needinfo?(rtilder)
Flags: needinfo?(rfkelly)
(In reply to Stephan Leroux [:sleroux] from comment #73)
> I noticed that the responses we get back while POSTing within a batch don't
> contain the 'modified' property but is included on the commit POST. :tcsc
> mentioned that this is ignored on Desktop where the X-Last-Modified property
> is used.

Actually, desktop is currently using x-weave-timestamp - see comment 68 above. So it sounds like we have *3* different signals for the same information - 2 headers and one field in the body :/

I can't recall the semantics I saw when playing with this, but desktop should *not* be looking at timestamps until commit time, even if those headers are present - and I'm also expecting all 3 timestamps are identical.
> So it sounds like we have *3* different signals for the same information

They happen to all be the same when you've written something, but they serve different purposes in other contexts.  But yes, the protocol specifies that they'll all identical in this case.
> If that turns out to be a PITA in practice, we can try to figure out another approach, but that's the thinking behind the current approach.

Nope it's fine. It's not a large change and I'd rather it be consistent across platforms. Even though X-Last-Modified and X-Weave-Timestamp are the same in this case, we should probably try to keep which one we use the same on all platforms. If desktop is using X-Weave-Timestamp I can do the same on iOS.
I gently suggest the opposite -- use XLM everywhere. It's clearer and more direct an expression of what it's being used for.
> I gently suggest the opposite -- use XLM everywhere. 

I less-gently agree with this - use X-Last-Modified everywhere, that's what it's for.
Depends on: 1304627
Depends on: 1311903
Depends on: 1315425
Priority: -- → P3
See Also: → 1320772
Depends on: 1332987
Assignee: rtilder → rfkelly
No longer depends on: 1250747
We shipped this, and there was much rejoicing!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Cross-client → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: