-
Notifications
You must be signed in to change notification settings - Fork 615
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
External watch API #2034
External watch API #2034
Conversation
api/store.proto
Outdated
StoreActionKind action = 1; | ||
|
||
// Matched object | ||
Object object = 2; |
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.
Can these be new
and old
or does that collide with a keyword?
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 that would be overly specific to the "update" case. For example, if we're looking at a deletion, new
wouldn't be the right field name for the object that was deleted.
Codecov Report
@@ Coverage Diff @@
## master #2034 +/- ##
==========================================
+ Coverage 54.28% 54.36% +0.07%
==========================================
Files 111 113 +2
Lines 19327 19448 +121
==========================================
+ Hits 10492 10572 +80
- Misses 7569 7601 +32
- Partials 1266 1275 +9 Continue to review full report at Codecov.
|
Rebased |
Is it easy to make
|
Good idea. Done in the most recent commit. Note that |
// Watch. Note that the first item of this stream will always be a WatchMessage | ||
// with a nil Object, to signal that the stream has started. | ||
message WatchMessage { | ||
message Event { |
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.
Each Event
should have a timestamp of commit time to store.
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.
Hm, this isn't currently available for historic events. For current Create
and Update
events, the individual objects will have the timestamp in Meta
(but Delete
events have the old timestamp).
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.
Let's use the Meta
timestamps.
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.
Should we omit it for delete events or include it even though it's the timestamp of the last update?
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.
Each event needs a timestamp. I'll use the last UpdatedAt
of delete events for now. Another option is to use the current time of machine.
@@ -0,0 +1,152 @@ | |||
syntax = "proto3"; |
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'd add a comment to everything that's not used
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.
Not sure I follow. This isn't used yet - it's being added so that events can use it.
// previous version of the object on updates. Note that only live | ||
// changes will include the old object (not historical changes | ||
// retrieved using ResumeFrom). | ||
bool include_old_object = 3; |
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.
Why do we need for this to be an option rather than the default?
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.
@dongluochen was concerned about wasted cycles and bandwidth sending two copies of the object for each change.
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.
For Event
, it need to include the old object to find the difference.
If an application just needs the current state and change as trigger, old object is not required. For example, if an application only cares about create
and remove
events. it doesn't need old object.
Rebased on #2039, and added code generation for all type-specific parts of this code. Now the only code in |
Rebased |
LGTM |
Ping @docker/core-swarmkit-maintainers. |
This is the first part of an external store API. Watch returns a stream of new, updated, or removed objects matching the given selectors. Signed-off-by: Aaron Lehmann <[email protected]>
This adds a Version to every WatchMessage, and a ResumeFrom field in the initial WatchRequest. A client can specify its last seen version to avoid doing a complete resync. Signed-off-by: Aaron Lehmann <[email protected]>
Signed-off-by: Aaron Lehmann <[email protected]>
Signed-off-by: Aaron Lehmann <[email protected]>
Signed-off-by: Aaron Lehmann <[email protected]>
Signed-off-by: Aaron Lehmann <[email protected]>
Signed-off-by: Aaron Lehmann <[email protected]>
Rebased |
@aaronlehmann do we filter secret |
No, this is not doing any redaction. It's the first piece of a store API, that will let external users interact with the data store at a lower level than the control API. I expect in the future there may be use cases where external controllers create and interact with secrets. We could always add redaction now, but might have to reconsider it later. |
message StoreObject { | ||
required WatchSelectors watch_selectors = 1; |
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.
is required
still a thing in proto3?
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.
This file doesn't use proto3
for whatever reason.
LGTM |
Add the first RPC from the external store API:
Watch
.This RPC supports resuming from a known location, as well as starting the watch stream to only receive future events.
It can optionally return the old version of the object as well as the new one for
Update
s.cc @dongluochen @aluzzardi