769500 - Implement batching APIs for StorageServiceClient

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Gregory Szorc [:gps]]

I want to add batching APIs to storage service client so callers can feed in or slurp out a large stream of BSOs without having to worry about chunking requests, etc.

This bug tracks implementing that.

Comment 1

[Richard Newman [:rnewman]]

Y'might be interested in the store batching in Server11RepositorySession.

https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/repositories/Server11RepositorySession.java#L343

Comment 2

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #1)
> Y'might be interested in the store batching in Server11RepositorySession.
> 
> https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/
> org/mozilla/gecko/sync/repositories/Server11RepositorySession.java#L343

Why would I want to copy a Java API ;)

Comment 3

[Gregory Szorc [:gps]]

Attachment: Implement batching uploading

Here is the first half: implementing batching uploads. This is the easier of the two parts.

I could probably submit this for r? and do a follow-up for retrieval functionality. If you want to upgrade the review, be my guest!

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 638061 [details] [diff] [review]
Implement batching uploading

Cancelling review because mutation requests to the same collection should be serialized, per rfkelly.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Implement batching set and delete

This implements APIs for batching set and delete. Basically, you can now perform these operations on unbounded streams of data. Very nice!

I have not yet implemented batching retrieve. That can be a follow-up bug or a part 2 or I can fold that work into this patch. Whatever.

I think the patch should be pretty self-explanatory as it is well-documented and I generally feel pretty good about the test coverage.

Comment 6

[Gregory Szorc [:gps]]

Things that came to mind as I took a walk:

* We are probably not recording all the state we need to perform adequate error reporting. I'm fine with waiting until I write a consumer of this API before we figure out what.

* There is a potential the code related to processing the queue could be made generic and factored out. When I write the batch fetching API, this might be obvious. I'm content with follow-ups. If nothing else, I won't have to rewrite the tests again :)

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 642806 [details] [diff] [review]
Implement batching set and delete

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

Needs a little more time.

::: services/common/storageservice.js
@@ +1086,5 @@
>   */
>  function StorageCollectionSetRequest() {
>    StorageServiceRequest.call(this);
>  
> +  this.size  = 0;

Double space.

@@ +1088,5 @@
>    StorageServiceRequest.call(this);
>  
> +  this.size  = 0;
> +
> +  // TODO convert to Set and Map once iterable.

Bug #.

@@ +1235,5 @@
> +
> +  this.locallyModifiedVersion = null;
> +  this.serverVersion          = null;
> +
> +  // TODO convert to Set and Map once iterable.

Same bug #.

@@ +1261,5 @@
> +    if (this._errorEncountered) {
> +      return;
> +    }
> +
> +    let added = this._stagingRequest.addBSO(bso, this.client.REQUEST_SIZE_LIMIT);

I'd rather see the thing that knows about bytes know what to do here.

@@ +1272,5 @@
> +
> +    if (!added) {
> +      this._log.debug("Sending request because payload limit exceeded.");
> +      this._finishStagedRequest();
> +      this._stagingRequest.addBSO(bso);

Write a test for a BSO bigger than the request size limit. Betcha it fails.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Implement batching set and delete, v2

Addressed feedback.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 644478 [details] [diff] [review]
Implement batching set and delete, v2

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

::: services/common/storageservice.js
@@ +1267,5 @@
> +    this._finished = true;
> +
> +    if (this._stagingRequest.count) {
> +      this._finishStagedRequest();
> +    }

Reuse this.flush().

@@ +1473,5 @@
> +      this._log.warn("Error received from server: " + error);
> +      this.errors.push(error);
> +    }
> +
> +    this.serverVersion = request.serverTime;

Wow, this use of the word "version" is misleading. Are you pulling this from the spec, or can we just change this to serverModified or something?

@@ +2031,5 @@
> +   *        (string) Collection to operate on.
> +   * @return
> +   *        (StorageCollectionBatchedSet) Batched set instance.
> +   */
> +  setBSOsBatching: function setBSOsBulk(collection) {

Function name agreement.

::: services/common/tests/unit/test_storageservice_client.js
@@ +26,5 @@
>      crypto:  {},
>    });
>  }
>  
> +function getClient(user=get_random_user(), password="password") {

Argh, why the mix between camelCase and under_scores? I prefer the latter for test code, but in this case I'd suggest s/get_random_user/getRandomUser/g.

@@ +970,5 @@
>    });
>  });
> +
> +add_test(function test_batching_set_too_large() {
> +  _("Ensure adding a BSO that is too large to fit throws.");

Sentence structure!

"Ensure that we throw when attempting to add a BSO that is too large to fit."

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/aa271df5f012

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/4f23bf1b45a9

Fixed an intermittent orange due to timing inconsistency.

Comment 12

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/aa271df5f012
https://hg.mozilla.org/mozilla-central/rev/4f23bf1b45a9