feat: add retry/timeout to manual surface #222

tseaver · 2020-10-13T19:34:14Z

Base Classes

Synchronous

Asynchronous

Unsure

base_collection.BaseCollection.on_snapshot (signature only)
base_document.BaseDocumentRef.on_snapshot (signature only)
base_query.BaseQuery.on_snapshot (signature only)
collection.Collection.on_snapshot
document.Document.on_snapshot
query.Query.on_snapshot
watch.Watch.__init__
watch.Watch.for_document
watch.Watch.for_query

Closes #221

Toward #221

…ents' Toward #221

Toward #221

…_all}' Toward #221

Toward #221

Methods include: - 'create' - 'set' - 'update' - 'delete' - 'get' - 'collections' Toward #221

Methods affected: - 'AsyncQuery.get' - 'AsyncQuery.stream' - 'AsyncCollectionGroup.get_partitions' Towards #221

Methods affected: - 'add' - 'get' - 'list_documents' - 'stream' Towards #221

Methods affected: - 'get_all' - 'get' Towards #221

tseaver · 2020-10-14T20:13:49Z

~~Note to reviewers: commit-by-commit is likely the most fruitful approach.~~

Updated: I just reviewed it as a whole, and think it is easier than following the commit-by-commit path.

tseaver

Comments on commit-by-commit review reflecting later changes as the PR evolved.

tseaver · 2020-10-14T20:42:16Z

google/cloud/firestore_v1/client.py

+        if timeout is not None:
+            kwargs["timeout"] = timeout
+
+        return kwargs


Later commits factor this method out into _helpers.make_retry_timeout_kwargs.

tseaver · 2020-10-14T20:43:58Z

tests/unit/v1/test_client.py

+            retry=retry,
+            timeout=timeout,
+            metadata=client._rpc_metadata,
+        )


40fae96 removes most of the redundancy in the tests for Client.get_all.

tseaver · 2020-10-14T20:45:15Z

tests/unit/v1/test_client.py

+            timeout=timeout,
+            metadata=client._rpc_metadata,
+        )
+


bad75e1 factors out the redundancy in the tests for Client.collections.

tseaver · 2020-10-14T20:45:48Z

google/cloud/firestore_v1/batch.py

+            kwargs["retry"] = retry
+
+        if timeout is not None:
+            kwargs["timeout"] = timeout


Since factored out to use _helpers.make_retry_timeout_kwargs.

tseaver · 2020-10-14T20:46:13Z

tests/unit/v1/test_batch.py

+            kwargs["retry"] = retry
+
+        if timeout is not None:
+            kwargs["timeout"] = timeout


Likewise, later commits update to use the helper here.

tseaver · 2020-10-14T20:55:29Z

tests/unit/v1/test_transaction.py

+            kwargs["retry"] = retry
+
+        if timeout is not None:
+            kwargs["timeout"] = timeout


Factored out later to use the helper.

tseaver · 2020-10-14T20:58:11Z

tests/unit/v1/test_client.py

+            kwargs["retry"] = retry
+
+        if timeout is not None:
+            kwargs["timeout"] = timeout


Factored out later to use the helper.

tseaver · 2020-10-14T20:58:59Z

tests/unit/v1/test_async_client.py

+            kwargs["retry"] = retry
+
+        if timeout is not None:
+            kwargs["timeout"] = timeout


Factored out later to use the helper.

tseaver · 2020-10-14T21:03:15Z

google/cloud/firestore_v1/async_document.py

        )

        while True:
            for i in iterator.collection_ids:
                yield self.collection(i)
            if iterator.next_page_token:
+                next_request = request.cpoy()


Typo fixed in 9b4707a

tseaver · 2020-10-14T21:04:13Z

google/cloud/firestore_v1/document.py

        )

        while True:
            for i in iterator.collection_ids:
                yield self.collection(i)
            if iterator.next_page_token:
+                next_request = request.cpoy()


Typo fixed in 1acbde3

google/cloud/firestore_v1/query.py

crwilcox · 2020-10-20T16:34:08Z

tests/unit/v1/test_async_client.py

        iterator = _Iterator(pages=[collection_ids])
        firestore_api.list_collection_ids.return_value = iterator

-        collections = [c async for c in client.collections()]
+        collections = [c async for c in client.collections(**kwargs)]


just confirming, you have added these changes to a few tests to get complete coverage? The changes of this PR shouldn't result in any required changes to users correct?

Right. See test_collections and test_collections_w_retry_timeout below.

Allows user to pass explicit 'retry=None' to suppress retries.

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 13, 2020

tseaver added 2 commits October 13, 2020 15:45

feat: add retry/timeout to 'client.Client.get_all'

8d03921

Toward #221

feat: add retry/timeout to 'client.Client.collections'

e7d2119

Toward #221

tseaver force-pushed the 221-retry-timeout branch from 1b61f49 to e7d2119 Compare October 13, 2020 19:46

tseaver added 14 commits October 13, 2020 16:25

feat: add retry/timeout to 'batch.Batch.commit'

6bfe32f

Toward #221

feat: add retry/timeout to 'document.DocumentReference.get'

6ba6d21

Toward #221

feat: add retry/timeout to 'query.Query.get'

e6ad4a1

Toward #221

feat: add retry/timeout to 'query.CollectionGroup.get_partitions'

6e806b0

Toward #221

feat: add retry/timeout to 'collection.CollectionReference.add'

00736fe

Toward #221

feat: add retry/timeout to 'collection.CollectionReference.list_docum…

2b80b91

…ents' Toward #221

feat: add retry/timeout to 'collection.CollectionReference.get'

49d3b03

Toward #221

feat: add retry/timeout to 'collection.CollectionReference.stream'

16a1e34

Toward #221

feat: add retry/timeout to 'document.DocumentReference.collections'

a9547e6

Toward #221

feat: add retry/timeout to 'document.DocumentReference.delete'

d6df19c

Toward #221

feat: add retry/timeout to 'document.DocumentReference.create'

6eae0e7

Toward #221

feat: add retry/timeout to 'document.DocumentReference.set'

559f2eb

Toward #221

feat: add retry/timeout to 'document.DocumentReference.update'

8038cce

Toward #221

feat: add retry/timeout to 'query.Query.stream'

2d413df

Toward #221

tseaver force-pushed the 221-retry-timeout branch from 07aa854 to 16a1e34 Compare October 13, 2020 21:38

tseaver added 11 commits October 13, 2020 18:01

feat: add retry/timeout to 'transaction.Transaction.get_all'

e15b8f6

Toward #221

feat: add retry/timeout to 'transaction.Transaction.get'

9f5bbb4

Toward #221

feat: add retry/timeout to base class signatures

5a1ef50

Toward #221

fix: un-break docs build

6dec6f3

chore: factor out helper for computing retry / timeout kwargs

812c41f

chore: factor out common prep for 'collections'/'get_all' to base class

8c67138

chore: factor out test helper for 'collections'

bad75e1

feat: add retry/timeout to 'async_client.AsyncClient.{collections.get…

db01b59

…_all}' Toward #221

chore: use factored-out helper to build retry/timeout kwargs

4090a00

chore: clean up tests for 'client.Client.get_all'

40fae96

chore: lint

df615e4

tseaver added 2 commits October 14, 2020 13:56

feat: add retry/timeout to 'async_batch.AsyncBatch.commit'

a557a15

Toward #221

feat: add retry/timeout to 'async_document.AsyncDocument` methods

4e3be50

Methods include: - 'create' - 'set' - 'update' - 'delete' - 'get' - 'collections' Toward #221

tseaver force-pushed the 221-retry-timeout branch from d6e76d6 to 4e3be50 Compare October 14, 2020 18:21

tseaver added 8 commits October 14, 2020 14:49

feat: add retry/timeout to 'async_query.Async{Query,CollectionGroup}'

ec8002c

Methods affected: - 'AsyncQuery.get' - 'AsyncQuery.stream' - 'AsyncCollectionGroup.get_partitions' Towards #221

feat: add retry/timeout to 'async_collection.AsyncCollectionReference'

c81cd8c

Methods affected: - 'add' - 'get' - 'list_documents' - 'stream' Towards #221

fix: typo

1acbde3

chore: rename testcases/helper for clarity

b998db2

feat: add retry/timeout to 'async_transaction.AsyncTransaction' methods

46f27e6

Methods affected: - 'get_all' - 'get' Towards #221

fix: typo

9b4707a

chore: appease pytype

7a976a5

fix: actually test retry / timeout

4b1ec26

tseaver marked this pull request as ready for review October 14, 2020 20:12

tseaver requested review from a team and chrisrossi October 14, 2020 20:12

tseaver commented Oct 14, 2020

View reviewed changes

crwilcox reviewed Oct 20, 2020

View reviewed changes

tseaver added 4 commits October 21, 2020 15:30

fix: document system-specified defaults for 'retry' / 'timeout'

c5e4056

fix: use gapic's 'DEFAULT' sentinel for 'retry'

2360eac

Allows user to pass explicit 'retry=None' to suppress retries.

Merge branch 'master' into 221-retry-timeout

f15a523

chore: lint

3feb63b

tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 21, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 21, 2020

crwilcox approved these changes Oct 21, 2020

View reviewed changes

tseaver merged commit db5f286 into master Oct 21, 2020

tseaver deleted the 221-retry-timeout branch October 21, 2020 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add retry/timeout to manual surface #222

feat: add retry/timeout to manual surface #222

tseaver commented Oct 13, 2020 •

edited

tseaver commented Oct 14, 2020 •

edited

tseaver left a comment

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

tseaver Oct 14, 2020

crwilcox Oct 20, 2020

tseaver Oct 21, 2020

feat: add retry/timeout to manual surface #222

feat: add retry/timeout to manual surface #222

Conversation

tseaver commented Oct 13, 2020 • edited

Base Classes

Synchronous

Asynchronous

Unsure

tseaver commented Oct 14, 2020 • edited

tseaver 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

tseaver commented Oct 13, 2020 •

edited

tseaver commented Oct 14, 2020 •

edited