-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Support blocking queries #366
Conversation
allocWatch: args.NodeID, | ||
queryOpts: &args.QueryOptions, | ||
queryMeta: &reply.QueryMeta, | ||
watch: watch.NewItems(watch.Item{AllocNode: args.NodeID}), |
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 wasn't sure if we would want to also watch the node entry here. It's possible to do that with the framework used here, but we would increase the frequency this watch would fire for potentially no change.
❤️ |
There is definitely common functionality here which we can abstract and share between both Consul and Nomad. MemDB would be a good place to expose the low-level change notification, and we can just implement the delta watch triggering in each respective project we need it for! Sounds great to me, this will just be for 0.2 or until we find a good way of exposing the functionality in a way that's useful for the greater good. |
Totally agree we could share this and MemDB seems like the right place. If MemDB handles table-level triggering with some data to inspect about what changed then the project-level stuff I think would be filtering and aggregation. Like MemDB calls a registered hook and gives you a list of keys added/removed or something like that. Would need to think on it a little more. For Consul making the watches more granular would be the perfect time to do this. I like how @ryanuber you've exposed some pretty low-level stuff up to the RPC call site to let the caller pick what they want. I kind of went back and forth on this and ended up having a mapping to state_store functions (kind of like the pre-MemDB code) but this way is probably better. |
LGTM |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This adds blocking queries to all of the RPC endpoints. This transitively allows blocking queries to be supported by all of the HTTP endpoints, except the /v1/agent/* ones. All of the blocking queries are scoped to minimize waking clients when there is no data change, so for example a blocking query on /v1/node/node1/allocations will not fire if a new allocation is made on node2.