Allowing 101 listeners #256

schmidt-sebastian · 2018-07-12T16:31:28Z

The GAPIC client that handles all of Firestore's networking is limited to 100 connections. This PR lift this limit by replacing the static GAPIC client with a pool of clients. If more than 100 operations run concurrently, we create additional clients.

codecov · 2018-07-12T17:12:31Z

Codecov Report

Merging #256 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #256   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        2104   2119   +15     
  Branches      457    458    +1     
=====================================
+ Hits         2104   2119   +15

Impacted Files	Coverage Δ
src/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9552c32...0aeee35. Read the comment docs.

test/util/helpers.ts

+    this.promise = new Promise(
+        (resolve: (value?: R|Promise<R>) => void,
+         reject: (reason?: Error) => void) => {
+          this.resolve = resolve;


src/pool.ts

+  }
+
+  /**
+   * Reduces the number of operation for the provided client, potentially


test/pool.ts

@@ -0,0 +1,158 @@
+/**
+ * Copyright 2017 Google Inc. All Rights Reserved.


test/pool.ts

+
+    const deferred = deferredPromises(4);
+
+    clientPool.run(() => deferred[0].promise);


test/pool.ts

+    completionPromises.push(clientPool.run(() => operationPromises[3].promise));
+    expect(clientPool.size).to.eq(2);
+
+    operationPromises[0].resolve();


schmidt-sebastian · 2018-07-12T20:41:23Z

Mike, since you have more context on this client, do you mind taking a look at this as well?

mikelehen

I got 99 problems, but listening to 1 more problem isn't one of them anymore.

This looks good to me. Nice work with the auto-resizing pool stuff. I left a few nit / comment suggestions, but nothing major.

src/index.js

@@ -126,6 +132,12 @@ const CLOUD_RESOURCE_HEADER = 'google-cloud-resource-prefix';
 */
 const MAX_REQUEST_RETRIES = 5;

+/*!
+ * The maximum number of concurrent requests supported by a GAPIC client.


src/index.js

@@ -239,11 +251,11 @@ class Firestore {
    });

    /**
+     * A client pool to distribute requests over multiple GAPIC clients.


src/pool.ts

+ * concurrent operations.
+ */
+export class ClientPool<T> {
+  private activeClients: Map<T, number> = new Map();


src/pool.ts

+    this.activeClients.forEach((requestCount, client) => {
+      if (!selectedClient && requestCount < this.concurrentOperationLimit) {
+        selectedClient = client;
+        currentRequestCount = requestCount;


src/pool.ts

+   */
+  private release(client: T): void {
+    let currentRequestCount = this.activeClients.get(client);
+    assert(currentRequestCount! > 0, 'Active client not found');


src/pool.ts

+   * removing it from the pool of active clients.
+   */
+  private release(client: T): void {
+    let currentRequestCount = this.activeClients.get(client);


src/pool.ts

+  run<V>(op: (client: T) => Promise<V>): Promise<V> {
+    const client = this.acquire();
+
+    return op(client)


src/pool.ts

+
+  /**
+   * Deletes clients that are no longer executing operations. Keeps up to one
+   * idle client to reduce future initialization costs.


system-test/firestore.js

+  version = require('../../package.json').version;
+} catch (e) {
+  version = require('../package.json').version;
+}


Previously _initializeStream returned a Promise that indicated that the stream was "released", i.e. that it was was ready for attaching listeners. #256 Added pooled clients and changed the callers of _initializeStream to reuse this promise such that when it was resolved, the stream could be returned to the pool. This works when listeners are short-lived, but fails when listeners run indefinitely. This change arranges to release the clients back to the pool only after the stream has completed, which allows an arbitrary number of indefinite listens to run without problems.

Fixes firebase/firebase-admin-node#499 Previously _initializeStream returned a Promise that indicated that the stream was "released", i.e. that it was was ready for attaching listeners. #256 Added pooled clients and changed the callers of _initializeStream to reuse this promise such that when it was resolved, the stream could be returned to the pool. This works when listeners are short-lived, but fails when listeners run indefinitely. This change arranges to release the clients back to the pool only after the stream has completed, which allows an arbitrary number of indefinite listens to run without problems. This turns out to be fiendishly difficult to test given the current structure of the code. A second pass at this that reformulates this as just another stream that composes with the others would make this easier to understand and test. For now, this fix unblocks the customers waiting on the referenced issue.

schmidt-sebastian added 4 commits July 11, 2018 08:41

Adding TypeScript dev dependencies

d30dfd2

Allowing tests to be run from /src

b72259e

Merge

d24cbca

Test initialization refactor

f4cb701

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 12, 2018

ghost assigned schmidt-sebastian Jul 12, 2018

schmidt-sebastian force-pushed the 101listeners branch from 0f4e688 to 331d3ec Compare July 12, 2018 17:12

Allowing 101 listeners

f1dce39

schmidt-sebastian force-pushed the 101listeners branch from 331d3ec to f1dce39 Compare July 12, 2018 17:19

var-const approved these changes Jul 12, 2018

View reviewed changes

schmidt-sebastian added 2 commits July 12, 2018 13:33

Review comments

b567f16

Fixing lint

9ee3979

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Jul 12, 2018

mikelehen approved these changes Jul 12, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Jul 12, 2018

Addressing Mike's comments

b3da6b6

schmidt-sebastian changed the base branch from initrefactor to master July 13, 2018 00:38

schmidt-sebastian force-pushed the 101listeners branch 2 times, most recently from 3ecd80b to e13acbe Compare July 13, 2018 00:46

Master merge

0aeee35

schmidt-sebastian force-pushed the 101listeners branch from e13acbe to 0aeee35 Compare July 13, 2018 00:47

schmidt-sebastian merged commit 4b853af into master Jul 13, 2018

schmidt-sebastian deleted the 101listeners branch July 24, 2018 04:26

wilhuff mentioned this pull request May 10, 2019

fix: Fix client pooling for long-lived listens #614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowing 101 listeners #256

Allowing 101 listeners #256

schmidt-sebastian commented Jul 12, 2018 •

edited

Loading

codecov bot commented Jul 12, 2018 •

edited

Loading

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

schmidt-sebastian commented Jul 12, 2018

mikelehen left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

		@@ -0,0 +1,158 @@
		/**
		* Copyright 2017 Google Inc. All Rights Reserved.


		const deferred = deferredPromises(4);

		clientPool.run(() => deferred[0].promise);

Allowing 101 listeners #256

Allowing 101 listeners #256

Conversation

schmidt-sebastian commented Jul 12, 2018 • edited Loading

codecov bot commented Jul 12, 2018 • edited Loading

Codecov Report

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

schmidt-sebastian commented Jul 12, 2018

mikelehen left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

schmidt-sebastian commented Jul 12, 2018 •

edited

Loading

codecov bot commented Jul 12, 2018 •

edited

Loading