add code snippet for Watch API #137
Conversation
9b3e0b0 to
f915ccd
Compare
tstirrat15
left a comment
There was a problem hiding this comment.
Can we either put it in the examples directory or additionally put it in the examples dir? https://github.com/authzed/authzed-java/tree/main/examples/v1
|
@tstirrat15 why "additionally"? What value does having it in the "examples" directory bring? Personal opinion, but it seems quite hidden |
f915ccd to
393cf64
Compare
README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| ### Calling `Watch` |
There was a problem hiding this comment.
Can we perhaps move this to the examples directory so it can be compiled and updated as needed? Then we can link the example from the readme. Same for check permissions.
README.md
Outdated
| builder.setOptionalStartCursor(lastZedToken); | ||
| } | ||
|
|
||
| WatchRequest request = builder.build(); |
There was a problem hiding this comment.
I'd suggest enabling heartbeats. This solves the confusion around idle timeouts and long-lived gRPC streaming APIs.
There was a problem hiding this comment.
I need to update the API first.
README.md
Outdated
|
|
||
| } catch (Exception e) { | ||
| if (e instanceof StatusRuntimeException sre && sre.getStatus().getCode().equals(Status.UNAVAILABLE.getCode()) && | ||
| (sre.getMessage().contains("stream timeout") || sre.getMessage().contains("RST_STREAM closed stream"))) { |
There was a problem hiding this comment.
We shouldn't be checking the message to trigger retries - these could be subject to change. I assume those have INTERNAL grpc code, so those are safe to be retried.
In addition to this manual handling, we could add retry policies, which is built into the Java gRPC SDK. Please note that retry policies are only supported for server-streaming APIs (like the Watch API), but are not supported for client-streaming and bidirectional APIs. I'm also not fully sure server-streaming is retried once data has been streamed back to the client.
|
@miparnisari i realize that it isn't often a place you would look, but when it's in a |
a13c387 to
fa39172
Compare
7add8f9 to
6095dc4
Compare
6095dc4 to
f239482
Compare
|
|
||
| } catch (Exception e) { | ||
| if (e instanceof StatusRuntimeException sre && (sre.getStatus().getCode().equals(Status.UNAVAILABLE.getCode()) || | ||
| (sre.getStatus().getCode().equals(Status.INTERNAL.getCode())) && sre.getMessage().contains("stream timeout"))) { |
There was a problem hiding this comment.
IMO we should just do this on the Internal code alone
There was a problem hiding this comment.
shouldn't we also retry on Unavailable?
ecordell
left a comment
There was a problem hiding this comment.
LGTM, with the caveat I haven't run this locally myself. But we can follow up if we find issues.
Description
Streaming APIs such as Watch can get disconnected. This PR provides a code snippet that can be used to automatically re-connect.
Testing
Locally, with https://github.com/miparnisari/authzed-java-client-test