feat: add retry/timeout to manual surface by tseaver · Pull Request #222 · googleapis/python-firestore

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

+        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

+            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

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


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

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

+            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

+            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

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


Factored out later to use the helper.

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

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


Factored out later to use the helper.

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

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


Factored out later to use the helper.

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

            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

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


Typo fixed in 1acbde3

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

        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

Uh oh!

Conversation

tseaver commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Base Classes

Synchronous

Asynchronous

Unsure

Uh oh!

tseaver commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tseaver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tseaver commented Oct 13, 2020 •

edited

Loading

tseaver commented Oct 14, 2020 •

edited

Loading