Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: retry and retry_async support streaming rpcs #495
feat: retry and retry_async support streaming rpcs #495
Changes from 83 commits
953106a
89aeb75
27feb80
0dffa6d
b330c3b
67ceaa2
ee2647a
5a5396c
7afa76b
2d91ade
0cd384e
f72bbec
88eed5c
91f9cc4
c3eb997
f6c6201
57b0ee3
e814ce7
0ffb03f
a8024f3
c76f641
ee631e3
70eb78c
42ee132
102d83b
f029dbd
c83c62a
185826c
c5f7bbe
4242036
9c4799c
0bd6cab
67aeeaf
985b13a
0ea8297
b952652
6cb3e2d
99da116
7f862d0
04a4a69
d20cf08
183c221
d2217e4
d4a9d30
06d45cc
de41a14
dcb3766
dd368e4
452b9bb
6879418
847509f
b5e3796
7a7d9ac
6619895
27fc930
d6a23ea
90ef834
6201db6
61ce3a7
69149a1
d63871e
773e033
d1def5d
cbaaa1d
21a863f
878ddfb
7b0a600
0188228
902a4ab
74f3f3e
e506aad
5baa2aa
5c3805d
265d998
0423ebe
c4049f5
acd6546
b1ad4b3
8dcf67c
6104c59
43d0913
9ba7676
14c195c
de7b51a
4cdee6b
a526d65
ee2bbdd
5f82355
9900c40
2c2dcbe
3340399
de07714
67068ac
54325bc
bafa18b
2ae2a32
9cadd63
c9ef1d5
41c7868
30fccb9
a2b0e6c
4aa1ab4
8349424
ece5cf8
5ddda24
9e3ea92
3b06b3a
8bb6b0c
37c64a0
cee0028
3a7e5fa
ba6dc9f
0500b8b
1ccadb1
c312262
1fe57e0
4f09f29
06824b9
343157b
93f82cc
0915ca0
61e5ab5
51c125b
02604bc
6269db2
0dcd0de
54e9c81
2342910
eada0d7
ae2bf37
c8a4f26
2840b9f
82274a3
1594a17
9b0ddb0
8985127
60b20ab
237ca3d
a46c0f7
93727b7
796ae52
0688ffe
da048ab
80e5eb0
562079b
a0fecc5
8cc6ea9
e7a5cd4
02c12cc
03b1608
b05b11f
0b5d3a2
03f2af5
5fee888
239ed7d
94eb0f5
7d1e246
b0faa2d
6c44298
51df672
e207376
39716a7
2bbf33f
3b03bfa
e63701d
c101ea6
3642d74
34cfa08
583181d
b311b87
19a998d
5637e88
c4be5f2
4d9e762
2e9e84b
d183a7e
e2d9c9c
4543106
d791aad
638cc68
f7b1e14
07db4c2
d448a52
781426a
4a05404
b221c8d
b5b4534
0f1145d
8408512
aa69c56
d1ac29d
3ab88fc
382d0e2
4258823
1bc9731
aafe057
8095229
de9f518
7864667
4c24322
7855513
f4bfb02
a88cf6f
b5c62e1
852f4f8
cd8323e
ace61eb
1bbd1f0
35cc00a
89abfa4
74ab817
85b3e02
6dbe17d
71e5888
cbae3d3
61198b8
acf9752
7cf9fbf
f62439a
b7abeca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In
retry_async.py:AsyncRetry
you have different type specifiers:timeout (Optional[float])
on_error (Optional[Callable[Exception]])
Should they be the same in both places? And it doesn't seem that the type hints in the
__init__
s quite match, either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah they probably should, I think I was just avoiding changing too much of the existing comments
If we're going to make a change, maybe we should remove the types from the comments entirely now that we have type annotations, so we don't have to duplicate it in multiple places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restart the stream from the last non-failed output, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that's not really something we can do in a general way. We can create a new generator and start yielding from it again, but we have no guarantee that it will yield the same sequence as last time. (For example, in Bigtable we will modify the request after a failure to avoid requesting repeat data from the server)
I considered adding a "filter" on top of the generator, and providing a default filter that raises an exception if a retry deviates from the initial stream, which would be one way to do what you requested. But in the end, I decided that it's probably better to keep this code simple and just pass on the generator values directly, and the caller can do their own filtering over top of the retry-stream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this functionality is not intended for external users directly, but rather for the library generator and handwritten library code, this seems fine. But the generator and library developers need to be aware of this and design their
target
andon_error
callables carefully if they want to accomplish certain behaviors such as the "filter through modified request" approach you described.Please expand the docstring of
RetryableGenerator
to provide some guidance and example pattern for the library developers so they do not make false assumptions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I added a much more documentation. Let me know that looks better
So far this will mostly just be useful for hand-written layers. If we want to add this to gapic libraries automatically, we could try to classify the different kinds of streams our libraries return and provide a couple different presets for different kinds of streams. But so far that's been out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the streaming versions have type declarations. Do you want to have type declarations in the non-stream versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I was trying to avoid scope growth, but it probably makes sense to add at this point. Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, the sleep-delay logic should be the same between all four versions of this function: syncvs async, unary vs streaming. I would suggest:
at the very least making the four functions look as similar as possible with their timeout handling logic. For example, the async versions uses
min()
, which the sync version probably should as well. The streaming functions useexception_factory
; should the unary ones as well, for consistenct?consider whether it's possible and desirable to factor any of the time- and exception-handling functionality into a common function. I suspect the answer is "no" because it would make the code more complex and harder to follow, but it would have the advantage that we'd be using the same logic in the parts where it makes sense. Maybe just factor out the
if deadline
and_LOGGER
at the bottom of the loop into a helper function that all four versions call to increment the sleep consistently?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to avoid making changes to the existing retries, but I suppose with the new base classes, it makes sense to generalize them a bit. I added a shared
_retry_error_helper
function, which either raises an error, or logs the retry message.Note though that I had to make a (potentially breaking) change to the async unary retry logic to make it consistent though. Previously async_unary would attempt to cancel early when the deadline is reached, while the other retries only check the deadline after the request terminates. The old behavior was very slow (using asyncio.wait_for), and caused race conditions. I'd consier it a bug, but fixing the behavior may count as a breaking change (even though gapic functions shouldn't be affected)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't use a function pointer here, like you did in the sync case (
retry_func = retry_target_stream if self._is_stream else retry_target
), so that we only need one wrapped function instead of two that are almost identical?NB: I notice that you're making changes to this PR, so I can't find the code that I quoted in the current version, though it is in the PR that I cloned locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only reason is because the existing
retry_wrapped_func
was an async function, and I was trying to add functionality without changing too much of the existing code.But I just made a change to make
retry_wrapped_func
into a sync function that returns the retry coroutine directly instead of awaiting it, and that makes things cleaner. So let me know if that worksThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new structure