Add requestProgress function to Watch and Watcher interfaces #957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
juliankung-db
force-pushed
the
progressNotifyRequest
branch
2 times, most recently
from
d62d2e4
to
eef02e7
Compare
juliankung-db
commented
Jul 29, 2021
| @Override | ||
| public void requestProgress() { | ||
| if (!closed.get()) { | ||
| synchronized (this.lock) { |
There was a problem hiding this comment.
Alternatively we can send a single RequestProgress update at this point (we'd need to add a FutureStub property if we go this route) and we can propagate the response to all watchers.
This'll save the etcd server from getting hit with multiple requests if there are multiple watchers
mikehuston
reviewed
Jul 29, 2021
docs/Watch.md
Outdated
| @@ -44,6 +46,13 @@ Cancel the watch task with the watcher, the `onCanceled` will be called after su | |||
| 1. The watcher will be removed from [watchers](#watchers-instance) map. | |||
| 2. If the [watchers](#watchers-instance) map contain the watcher, it will be moved to [cancelWatchers](#cancelwatchers) and send cancel request to [requestStream](#requeststream-instance). | |||
|
|
|||
| ## requestProgress function | |||
|
|
|||
| Send the most updated revision number to all active [watchers](#watchers-instance) | |||
There was a problem hiding this comment.
Send the latest revision processed by each watcher to all... ?
There was a problem hiding this comment.
Changed to be similar to what you suggested
docs/Watch.md
Outdated
| # Implementation | ||
|
|
||
| The etcd client process watch request with [watch function](#watch-function), process notification with [processEvents function](#processevents-function) , process resume with [resume function](#resume-function) and process cancel with [cancelWatch function](#cancelwatch-function). | ||
| The etcd client process watch request with [watch function](#watch-function), process notification with [processEvents function](#processevents-function) , process resume with [resume function](#resume-function), process cancel with [cancelWatch function](#cancelwatch-function) and process progress with [requestProgress function](#requestProgress-function). |
There was a problem hiding this comment.
Suggested change
| The etcd client process watch request with [watch function](#watch-function), process notification with [processEvents function](#processevents-function) , process resume with [resume function](#resume-function), process cancel with [cancelWatch function](#cancelwatch-function) and process progress with [requestProgress function](#requestProgress-function). | |
| The etcd client process watch request with [watch function](#watch-function), process notification with [processEvents function](#processevents-function) , process resume with [resume function](#resume-function), process cancel with [cancelWatch function](#cancelwatch-function) and [requestProgress function](#requestProgress-function). |
There was a problem hiding this comment.
Changed to be more clear
mikehuston
reviewed
Jul 29, 2021
| // For watch progress responses, the header.revision indicates progress. All future events | ||
| // received in this stream are guaranteed to have a higher revision number than the | ||
| // header.revision number. | ||
| rpc Progress(WatchProgressRequest) returns (WatchResponse) {} |
There was a problem hiding this comment.
Suggested change
| rpc Progress(WatchProgressRequest) returns (WatchResponse) {} | |
| rpc ProgressRequest(WatchProgressRequest) returns (WatchResponse) {} |
There was a problem hiding this comment.
I'll leave this for the maintainers to decide. I was following what looked like the proto pattern for RPCs in the file
There was a problem hiding this comment.
I'd go with Progress so it uses the same pattern
Added a function that requests a watch stream progress status be sent in the watch response stream as soon as possible. This is helpful in situations where an application may want to check the progress of a watch to determine how up-to-date the watch stream is. Addresses etcd-io#928
juliankung-db
force-pushed
the
progressNotifyRequest
branch
from
eef02e7
to
157cf38
Compare
lburgazzoli
approved these changes
Jul 30, 2021
|
@lburgazzoli would you mind merging when you have the chance? I don't have write access so I can't do it myself |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Added a
requestProgressfunction to theWatchandWatcherinterfaces to request a WatchResponse be sent to all watchers containing the latest revision number. This can be used to check if a watcher is "caught up" to a particular revision, which is useful when determining if a watch cache is stale by comparing the progress revision to the revision returned in a quorum read.This was added to the etcd client in etcd-io/etcd#9869 and requested for the jetcd client in #928
Commit message below
Added a function that requests a watch stream progress status
be sent in the watch response stream as soon as possible. This is
helpful in situations where an application may want to check the
progress of a watch to determine how up-to-date the watch stream is.
Addresses #928