Implement limbo resolution throttling. #2790

dconeybe · 2020-03-24T17:39:29Z

This change was motivated by the following bug report: #2683. In summary, the issue was that when a large number of documents went into limbo (in this case 15,000 documents) then this would cause 15,000 document listens, which would result in "resource-exhausted" errors. The client would then back off for a bit but would try the listens again, which would again result in "resource-exhausted" errors. And the cycle would continue. Worst of all, the customer was being billed for all of these reads that had no beneficial effects for the clients.

In order to avoid this situation, this PR modifies the limbo resolution logic to "throttle" the limbo resolutions. It does this by allowing at most 100 concurrent limbo resolutions. Limbo resolutions in excess of this limit are queued up and not started until another limbo resolution completes. This fix should avoid the "resource-exhausted" errors and the infinite read loop.

google-oss-bot · 2020-03-24T17:47:45Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`bd85150`)	Head (`6df1227`)	Diff
@firebase/firestore/memory	browser	226115.00	226315.00	+200.00 (+0.09%)
	module	223295.00	223495.00	+200.00 (+0.09%)
	esm2017	171948.00	172104.00	+156.00 (+0.09%)
	main	399467.00	400263.00	+796.00 (+0.20%)
@firebase/firestore	browser	272688.00	272888.00	+200.00 (+0.07%)
	module	269467.00	269667.00	+200.00 (+0.07%)
	esm2017	269467.00	269667.00	+200.00 (+0.07%)
	main	490193.00	490989.00	+796.00 (+0.16%)
firebase	firebase.js	847065.00	847267.00	+202.00 (+0.02%)
	firebase-firestore.memory.js	268418.00	268620.00	+202.00 (+0.08%)
	firebase-firestore.js	313659.00	313861.00	+202.00 (+0.06%)

Metric Unit: byte

Test Logs

wilhuff

Despite the volume of comments this actually looks great. A lot of the comments are more nitpicky than substantive.

One high-level thought though: limbo resolution is now somewhat complex (or at least more than before). There's a flow to the way documents go through limbo and that's not really documented anywhere. While it's possible to document this inline in the SyncEngine, pulling this process out into a separate LimboResolver class would give you a class comment as a place to tie this all together. If that refactoring doesn't result in any crazy indirection, it seems like it would also be helpful in reducing the number of things for which SyncEngine is directly responsible.

wilhuff · 2020-03-25T17:39:00Z

packages/firestore/src/core/sync_engine.ts

+    }
+  }
+
+  private startEnqueuedLimboResolutions(): void {


This name is off:

we tend to use "start" and "stop" for beginning and ending continuous processes, like startWatchStream, after which the watch stream has started until we stop it and

this method doesn't necessarily start any resolutions if the number of pending resolutions is at the maximum.

In the RemoteStore there's a fillWritePipeline that has a similar high level behavior. The analogous name would be fillLimboResolutionPipeline, but while it's an improvement I can't say I think this is an obvious winner, probably because we don't explicitly have a pipeline the way the remote store does. The symmetry is appealing though.

Other ideas:

drainLimboResolutionQueue is problematic because it suggests waiting for all the queued resolutions.

I've seen "pump" used for this kind of operation.

fillActiveLimboResolutions? (building on pending vs active, suggested elsewhere)

All things considered I'd probably just go with fillLimboResolutionPipeline for symmetry with RemoteStore more than anything but I'm definitely open to alternatives that avoid "start".

Also, please add comments. The comments above fillWritePipeline are a good example: what the thing does and when it should be used.

I will add the requested documentation to the method.

Regarding the naming issue... what about using the verb "trigger" in the method name? The "pipeline" idea makes sense too, but the method in question is not technically "filling" the pipeline and I find that verb to be misleading. The "pump" verb is sounding good to me though. According to https://en.wikipedia.org/wiki/Event_loop the verb "pump" has a meaning in an event loop/message queue context, which has many analogues to the limbo resolution queue. That article also suggests the verb "dispatch".

So here are some other names to consider:

triggerLimboResolutions()

pumpLimboResolutionQueue() (my preference)

dispatchLimboResolutionListens()

Regarding the limbo resolution state names, I'm leaning towards the terms "enqueued" and "active". I prefer "enqueued" over "pending" because, IMO, it is a more concrete term than "pending" and therefore requires less explanation in documentation.

What you implemented was pumpLimboResolutionListenQueue. I offer that you should construct this as "pump" plus the name of the thing. If you go with enqueuedLimboResolutions, above, then match with pumpEnqueuedLimboResolutions.

I'm also fine with "trigger" if that reads better to you.

Done. I've renamed the method to pumpEnqueuedLimboResolutions().

wilhuff · 2020-03-25T17:46:52Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */


There are now a number of limbo-related structures in here and it would be worthwhile to document them in relation to one another and give an overview of how they're supposed to fit together.

Also, since things start in the limboListenQueue, I suggest making it structurally first among these. That way, when reading through this list, the structure suggests the flow.

Also, this specific comment doesn't do much to suggest what the queue is actually for. It's worth calling out that these are waiting to start a the query. Maybe something like this?

The keys of documents that are in limbo for which we haven't yet started a limbo resolution query.

I've added/improved the documentation for limboListenQueue, limboTargetsByKey, and limboResolutionsByTarget. I don't have enough context to provide meaningful documentation for limboDocumentRefs. Since this PR does not touch anything related to limboDocumentRefs I've left it alone. I also moved limboListenQueue in the code to be structurally first.

wilhuff · 2020-03-25T17:48:01Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */
+  private limboListenQueue: DocumentKey[] = [];


What's the difference between a "limbo listen" and a "limbo resolution"? Is there any? If not I suggest sticking with one name to reduce the conceptual load here.

A limbo "resolution" is the entire process that begins when a document enters the limbo state and ends when the limbo state is resolved (i.e. after getting a response from the server). A "listen" is the thing that the remoteStore does when we call remoteStore.listen(...). I agree, however, that the terminology is confusing and I'll make any changes needed to be consistent.

I'm still concerned about the "listen" in limboListenQueue as a name here. Even your comment doesn't mention "listens" so it's not clear if there's an actual conceptual difference between limbo resolution and a limbo listen or if this is just an accidental inconsistency that wasn't caught in review.

Since you've resolved to distinguish active and enqueued limbo resolutions, why can't this be enqueuedLimboResolutions? That is: instead of focusing on the fact that they're enqueued to start a listener, just know that these are limbo resolutions in the enqueued state without specifying exactly what they're waiting for. This resolves the inconsistency without any effective loss of clarity, because exactly what's next after enqueueing isn't a critical detail in this name.

If you prefer "Queue" in the name, "deferred" might better name for the enqueued resolutions because then you can make this deferredLimboResolutionQueue. enqueuedLimboResolutionQueue looks strange.

Done. I've renamed the variable to enqueuedLimboResolutions.

wilhuff · 2020-03-25T17:55:09Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */
+  private limboListenQueue: DocumentKey[] = [];
  private limboDocumentRefs = new ReferenceSet();


Many of the existing variable names are now ambiguous because it's not clear if they apply to active limbo resolutions, inactive limbo resolutions, or all. I suggest prefixing all these names to clearly indicate which kind each is.

Other ideas for ways to clarify this:

active vs pending (though "pending" could also possibly be read to mean the resolution for which we're waiting on a result)

active vs deferred?

If we document this distinction though I think any pair could be good and an improvement. WDYT?

I like the names "enqueued" and "active" for the states of documents that are in limbo. Since a limbo resolution is "active" if and only if it is associated with a target, the limboTargetsByKey and limboResolutionsByTarget imply that they only apply to "active" limbo resolutions. Similarly, a limbo resolution is "enqueued" if and only if its document key is in the limboListenQueue. My sense is that adding "active" and "inactive" to these variable names will be confusing because they have no counterpart for the opposite state.

variable names will be confusing because they have no counterpart for the opposite state

That's an interesting perspective but I think the reasoning is kind of subtle: that because only active limbo resolutions can have a target the active-ness is self-explanatory. Would this have been self-evident to you a week ago?

My reasoning was that the prefix helps identify which variables are used in which state, and suggests which elements have to be modified as you transition between states. That is, this would have the effect of grouping these things together more than necessarily differentiating similar structures of the same type that might be used in the different states.

Makes sense. I've added "active" and "enqueued" to all relevant variable names throughout this PR.

wilhuff · 2020-03-25T18:07:05Z

packages/firestore/src/core/sync_engine.ts

-    private currentUser: User
-  ) {}
+    private currentUser: User,
+    maxConcurrentLimboResolutions?: number


This could be made simpler by just defaulting the argument.

private maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS

Done. I also removed the DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS constant and just inlined the value of 100. IMO this improves readability at no cost to performance or generality.

wilhuff · 2020-03-25T18:40:50Z

packages/firestore/test/unit/specs/limbo_spec.test.ts

+            added: [docB1, docB2, docB3],
+            fromCache: true
+          })
+          // After the existence filter mismatch the client re-listens without


Using "after" here makes this seem like the existence filter mismatch is happening elsewhere, but it's really happening here right? I suggest rephrasing this to something that more actively explains why this is going on:

The view now contains the docAs and the docBs (6 documents), but the existence filter indicated only three should match. This causes the client to re-listen without a resume token.

Done. I took your suggestion verbatim.

wilhuff · 2020-03-25T18:46:35Z

packages/firestore/test/unit/specs/limbo_spec.test.ts

+          .watchAcks(docA3Query)
+          .watchCurrents(docA3Query, 'resume-token-1004')
+          .watchSnapshots(1004)
+          // Watch has now confirmed that docA3 has been deleted. So the


These are good comments, but they're a little wordy. If you could make them more concise it would help the next reader focus on the interesting bits. For example:

"the listener sends an event about this" is mostly superfluous because we're always sending events.

Similarly, "since we are in sync with the server and all docs that were in limbo have been resolved" is at least self-duplicating because "in sync" implies no documents in limbo.

you could shorten it like so:

Watch has confirmed the final limbo document has been deleted so expect an event with fromCache=false, indicating we're in sync again.

I realize this is something of a tall order because it's not obvious which behaviors are implicit, but give it a shot.

Done. Comments have been overhauled, as per our offline discussions.

wilhuff · 2020-03-25T18:53:22Z

packages/firestore/test/unit/specs/spec_builder.ts

@@ -353,7 +358,8 @@ export class SpecBuilder {
      enableNetwork: false,
      expectedState: {
        activeTargets: {},
-        limboDocs: []
+        limboDocs: [],


As above, limboDocs is now ambiguous. It's worth carrying the active/pending or similar distinction through to here so that it's consistent.

Done. Note that this causes a backwards incompatibility with the JSON test specs that are generated for Android and iOS. Once this PR is merged, I will submit PRs to those two repositories to use "activeLimboDocs" instead of "limboDocs".

wilhuff · 2020-03-25T18:57:24Z

packages/firestore/test/unit/specs/spec_builder.ts

+   * target.  For the limbo resolutions that should be active, a targetId will
+   * be assigned if it hasn't already been assigned.
+   */
+  expectLimboDocsEx(limboDocs: {


I'm not a super fan of the "Ex" convention. You see this a lot in long-lived stable APIs that can't be changed, especially in languages where overloads aren't possible (the example that comes to mind are Win32 C APIs). This is neither of those cases, so I suggest either:

just make this an overload of expectLimboDocs or

make expectLimboDocs only specify active limbo documents and add a separate expectInactiveLimboDocs.

I'd favor the latter, since it's simpler (in typescript, overloads exist for callers, but the implementation still has to manually disambiguate which variant is being called).

I agree with your suggestion of adding a new expectInactiveLimboDocs() method. I've made this change.

wilhuff · 2020-03-25T19:01:41Z

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -405,6 +406,7 @@ abstract class TestRunner {
  );

  private expectedLimboDocs: DocumentKey[];
+  private expectedInactiveLimboDocs: DocumentKey[];


Please resolve the ambiguity of the expectedLimboDocs name too.

wilhuff · 2020-03-25T19:21:20Z

Also, in the description, please indicate which issue this addresses and give an overview of how it address it.

dconeybe · 2020-03-26T14:18:10Z

@wilhuff I have updated the description of this PR, as requested. Thank you for the thorough review and excellent feedback!

Regarding your suggestion of moving the limbo resolution logic out of the SyncEngine class and into a new LimboResolver class... I really like this idea. I think it makes a great deal of sense for readability and maintainability. My only concern is that such a change should be in its own commit, rather than mixing refactoring and functionality change in a single commit. I could do the refactoring either before this PR or after it. I'm leaning towards after so that we can avoid delaying this fix and then do the refactoring after. What are your thoughts?

dconeybe

I left two comments outside of this review (because I just learned about "reviews" in GitHub today!).

dconeybe · 2020-03-26T16:29:43Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */


I've added/improved the documentation for limboListenQueue, limboTargetsByKey, and limboResolutionsByTarget. I don't have enough context to provide meaningful documentation for limboDocumentRefs. Since this PR does not touch anything related to limboDocumentRefs I've left it alone. I also moved limboListenQueue in the code to be structurally first.

dconeybe · 2020-03-26T16:31:52Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */
+  private limboListenQueue: DocumentKey[] = [];


A limbo "resolution" is the entire process that begins when a document enters the limbo state and ends when the limbo state is resolved (i.e. after getting a response from the server). A "listen" is the thing that the remoteStore does when we call remoteStore.listen(...). I agree, however, that the terminology is confusing and I'll make any changes needed to be consistent.

dconeybe · 2020-03-26T16:53:59Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */
+  private limboListenQueue: DocumentKey[] = [];
  private limboDocumentRefs = new ReferenceSet();


I like the names "enqueued" and "active" for the states of documents that are in limbo. Since a limbo resolution is "active" if and only if it is associated with a target, the limboTargetsByKey and limboResolutionsByTarget imply that they only apply to "active" limbo resolutions. Similarly, a limbo resolution is "enqueued" if and only if its document key is in the limboListenQueue. My sense is that adding "active" and "inactive" to these variable names will be confusing because they have no counterpart for the opposite state.

dconeybe · 2020-03-26T16:56:36Z

packages/firestore/src/core/sync_engine.ts

-    private currentUser: User
-  ) {}
+    private currentUser: User,
+    maxConcurrentLimboResolutions?: number


Done. I also removed the DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS constant and just inlined the value of 100. IMO this improves readability at no cost to performance or generality.

dconeybe · 2020-03-26T18:09:22Z

packages/firestore/test/unit/specs/limbo_spec.test.ts

+          // resume token.
+          .restoreListen(query, 'resume-token-1000')
+          .watchAcks(query)
+          // Watch now tells us that the query results on the server are docB1,


Done. I took your suggestion verbatim.

dconeybe · 2020-03-26T19:04:51Z

packages/firestore/test/unit/specs/limbo_spec.test.ts

+            added: [docB1, docB2, docB3],
+            fromCache: true
+          })
+          // After the existence filter mismatch the client re-listens without


Done. I took your suggestion verbatim.

dconeybe · 2020-03-26T19:05:20Z

packages/firestore/test/unit/specs/limbo_spec.test.ts

+          .watchAcks(docA3Query)
+          .watchCurrents(docA3Query, 'resume-token-1004')
+          .watchSnapshots(1004)
+          // Watch has now confirmed that docA3 has been deleted. So the


Done. Comments have been overhauled, as per our offline discussions.

dconeybe · 2020-03-26T19:12:56Z

packages/firestore/test/unit/specs/spec_builder.ts

@@ -353,7 +358,8 @@ export class SpecBuilder {
      enableNetwork: false,
      expectedState: {
        activeTargets: {},
-        limboDocs: []
+        limboDocs: [],


Done. Note that this causes a backwards incompatibility with the JSON test specs that are generated for Android and iOS. Once this PR is merged, I will submit PRs to those two repositories to use "activeLimboDocs" instead of "limboDocs".

dconeybe · 2020-03-26T19:17:40Z

packages/firestore/test/unit/specs/spec_builder.ts

+   * target.  For the limbo resolutions that should be active, a targetId will
+   * be assigned if it hasn't already been assigned.
+   */
+  expectLimboDocsEx(limboDocs: {


I agree with your suggestion of adding a new expectInactiveLimboDocs() method. I've made this change.

dconeybe · 2020-03-26T19:32:41Z

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -405,6 +406,7 @@ abstract class TestRunner {
  );

  private expectedLimboDocs: DocumentKey[];
+  private expectedInactiveLimboDocs: DocumentKey[];


wilhuff · 2020-03-27T17:55:46Z

packages/firestore/src/core/sync_engine.ts

@@ -148,9 +148,13 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
    q.canonicalId()
  );
  private queriesByTarget: { [targetId: number]: Query[] } = {};
+  /** The keys of documents that are in limbo for which we haven't yet started a limbo resolution query. */


80 columns. Here and below. Unfortunately, the formatter doesn't enforce this for comments.

Whoops. I had assumed that yarn prettier would take care of this.

wilhuff · 2020-03-27T18:10:19Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */
+  private limboListenQueue: DocumentKey[] = [];
  private limboDocumentRefs = new ReferenceSet();


variable names will be confusing because they have no counterpart for the opposite state

That's an interesting perspective but I think the reasoning is kind of subtle: that because only active limbo resolutions can have a target the active-ness is self-explanatory. Would this have been self-evident to you a week ago?

My reasoning was that the prefix helps identify which variables are used in which state, and suggests which elements have to be modified as you transition between states. That is, this would have the effect of grouping these things together more than necessarily differentiating similar structures of the same type that might be used in the different states.

wilhuff · 2020-03-27T18:12:18Z

packages/firestore/src/core/sync_engine.ts

@@ -154,6 +155,9 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
  private limboResolutionsByTarget: {
    [targetId: number]: LimboResolution;
  } = {};
+  private readonly maxConcurrentLimboResolutions: number = DEFAULT_MAX_CONCURRENT_LIMBO_RESOLUTIONS;
+  /** The keys of documents whose limbo resolutions are enqueued. */
+  private limboListenQueue: DocumentKey[] = [];


I'm still concerned about the "listen" in limboListenQueue as a name here. Even your comment doesn't mention "listens" so it's not clear if there's an actual conceptual difference between limbo resolution and a limbo listen or if this is just an accidental inconsistency that wasn't caught in review.

Since you've resolved to distinguish active and enqueued limbo resolutions, why can't this be enqueuedLimboResolutions? That is: instead of focusing on the fact that they're enqueued to start a listener, just know that these are limbo resolutions in the enqueued state without specifying exactly what they're waiting for. This resolves the inconsistency without any effective loss of clarity, because exactly what's next after enqueueing isn't a critical detail in this name.

If you prefer "Queue" in the name, "deferred" might better name for the enqueued resolutions because then you can make this deferredLimboResolutionQueue. enqueuedLimboResolutionQueue looks strange.

wilhuff · 2020-03-27T18:16:22Z

packages/firestore/src/core/sync_engine.ts

+    }
+  }
+
+  private startEnqueuedLimboResolutions(): void {


What you implemented was pumpLimboResolutionListenQueue. I offer that you should construct this as "pump" plus the name of the thing. If you go with enqueuedLimboResolutions, above, then match with pumpEnqueuedLimboResolutions.

I'm also fine with "trigger" if that reads better to you.

wilhuff · 2020-03-27T18:17:27Z

packages/firestore/src/core/sync_engine.ts

+   * Starts listens for documents in limbo that are enqueued for resolution.
+   *
+   * When a document goes into limbo it is enqueued for resolution. This method
+   * repeatedly removes entries from the limbo resolution queue and starts a


Your description here uses "limbo resolution queue". I think this supports renaming "limbo listen" to "limbo resolution".

Agreed. I will update this PR to use consistent terminology.

wilhuff · 2020-03-27T18:27:11Z

packages/firestore/src/core/sync_engine.ts

+   * (either successfully or unsuccessfully). This ensures that all documents in
+   * limbo are eventually resolved.
+   *
+   * A maximum number of concurrent limbo resolution listens was implemented to


This comment buries the lede: limiting concurrent limbo resolution is a top-line feature of this method. It also focuses on the change we made instead of focusing on documenting the intended behavior and what motivated that. In particular, it's not clear why "resource exhausted" errors are necessarily a problem. I suggest moving this into the first paragraph and emphasizing what this fixes. Something like this:

Starts listens for documents in limbo that are enqueued for resolution, subject to a maximum number of concurrent resolutions. Without bounding the number of concurrent resolutions, the server can fail with resource exhausted errors which can lead to pathological client behavior as seen in #2683.

Done. I like your comment a lot better. I've removed some of the extra explanation from the comments since they are bound to go out-of-date anyways with future changes to the sync engine.

wilhuff · 2020-03-27T18:29:09Z

packages/firestore/test/unit/specs/spec_builder.ts

+  /**
+   * Expects a document to be in limbo, but *without* a targetId.
+   */
+  expectInactiveLimboDocs(...keys: DocumentKey[]): this {


Elsewhere you're calling these "enqueued" limbo docs. Let's be consistent throughout.

Done. Do you think it is okay to leave the expectedLimboDocs() method with its current name? It does not explicitly call out the fact that it is referring to "active" limbo documents, but this is the typical case used in many places in the spec tests. Moreover, the difference between "enqueued" and "inactive" only matters here in the spec tests for limbo resolution throttling.

I can see this going two ways:

Just leave it as-is, which is pretty much fine

Make expectedLimboDocs check both active and inactive docs and make a third expectedActiveLimboDocs that just checks actives.

Personally, I think (2) is veering into overengineering territory so I'm OK with just leaving it.

I tend to agree. I'll leave it as-is, with expectedLimboDocs() being the name of the method.

…teEnqueuedLimboDocs

wilhuff

Very close to LGTM

wilhuff · 2020-03-30T16:27:18Z

packages/firestore/test/unit/specs/spec_test_runner.ts

-      this.sharedClientState,
-      this.user
-    );
+    if (this.maxConcurrentLimboResolutions) {


If there's more configuration knobs like this one, this kind of separate invocation for each configuration will explode in complexity. It's probably better to have a named constant, export it, and just use that default value here.

Alternatively, maybe set the default here to something obscenely huge to prevent limbo throttling altogether if it's not intended?

Agreed. Thanks for pointing this out because I didn't like it either. I've changed maxConcurrentLimboResolutions to a required parameter, delegating the responsibility to the caller to determine the value. I then updated firestore_client.ts to specify the value of 100 and used Number.MAX_SAFE_INTEGER in the spec tests, unless explicitly overridden by a specific test.

wilhuff · 2020-03-30T16:33:46Z

packages/firestore/test/unit/specs/spec_builder.ts

+  /**
+   * Expects a document to be in limbo, but *without* a targetId.
+   */
+  expectInactiveLimboDocs(...keys: DocumentKey[]): this {


I can see this going two ways:

Just leave it as-is, which is pretty much fine

Make expectedLimboDocs check both active and inactive docs and make a third expectedActiveLimboDocs that just checks actives.

Personally, I think (2) is veering into overengineering territory so I'm OK with just leaving it.

…gine constructor.

wilhuff

LGTM

This change to the change log was forgotten in the PR that added throttling: #2790

Implement limbo resolution throttling.

d8d7293

dconeybe assigned dconeybe and wilhuff and unassigned dconeybe Mar 24, 2020

dconeybe requested a review from wilhuff March 24, 2020 17:40

wilhuff suggested changes Mar 25, 2020

View reviewed changes

wilhuff assigned dconeybe and unassigned wilhuff Mar 25, 2020

wilhuff added the api: firestore label Mar 25, 2020

Implement code review feedback from @wilhuff, round 1

5900aa8

dconeybe commented Mar 26, 2020

View reviewed changes

dconeybe assigned wilhuff and unassigned dconeybe Mar 27, 2020

wilhuff reviewed Mar 27, 2020

View reviewed changes

wilhuff assigned dconeybe and unassigned wilhuff Mar 27, 2020

dconeybe added 4 commits March 30, 2020 11:51

Implement code review feedback from @wilhuff, round 2

f0a2077

[AUTOMATED]: License Headers

45802c3

Fix spelling error in method name: validateEnqeuedLimboDocs -> valida…

545fa89

…teEnqueuedLimboDocs

Merge remote-tracking branch 'origin/master' into LimboThrottling

abfa6fb

wilhuff reviewed Mar 30, 2020

View reviewed changes

Make maxConcurrentLimboResolutions a required parameter of the SyncEn…

4c1a20c

…gine constructor.

dconeybe assigned wilhuff and unassigned dconeybe Mar 30, 2020

wilhuff approved these changes Mar 30, 2020

View reviewed changes

wilhuff assigned dconeybe and unassigned wilhuff Mar 30, 2020

dconeybe merged commit 74b06f5 into master Mar 30, 2020

dconeybe deleted the dconeybe/LimboThrottling branch March 30, 2020 18:44

This was referenced Mar 30, 2020

Update spec tests from changes in the firebase-js-sdk respository firebase/firebase-android-sdk#1401

Merged

Update spec tests from changes in the firebase-js-sdk respository firebase/firebase-ios-sdk#5278

Merged

dconeybe added a commit that referenced this pull request Apr 3, 2020

Update CHANGELOG.md to mention limbo resolution throttling

42ad868

This change to the change log was forgotten in the PR that added throttling: #2790

This was referenced Apr 3, 2020

Update CHANGELOG.md to mention limbo resolution throttling #2864

Merged

Implement limbo resolution listen throttling. firebase/firebase-android-sdk#1374

Merged

Implement limbo resolution listen throttling firebase/firebase-ios-sdk#5310

Merged

firebase locked and limited conversation to collaborators Apr 30, 2020

Implement limbo resolution throttling. #2790

Implement limbo resolution throttling. #2790

Conversation

dconeybe commented Mar 24, 2020 • edited Loading

google-oss-bot commented Mar 24, 2020 • edited Loading

Binary Size Report

Affected SDKs

Test Logs

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff commented Mar 25, 2020

dconeybe commented Mar 26, 2020

dconeybe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

dconeybe commented Mar 24, 2020 •

edited

Loading

google-oss-bot commented Mar 24, 2020 •

edited

Loading