-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
events: Add filters to keep track of local and other subscriptions #24201
Conversation
This adds a very basic implementation of a list of namespace+eventType combinations that each node is interested in by just running the glob operations in for-loops. Some parallelization is possible, but not enabled by default. It only wires up keeping track of what the local event bus is interested in for now (but doesn't use it yet to filter messages).
b88a56f
to
a9f78be
Compare
Build Results: |
CI Results: |
…ions; local node is probably better anyway
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.
LGTM - just a few suggestions
vault/eventbus/filter.go
Outdated
patterns []pattern | ||
} | ||
|
||
func (nf *NodeFilter) match(nsPath string, eventType logical.EventType) bool { |
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.
It would be nice to accept *namespace.Namespace
instead of string
for all the nsPath
parameters in this file - that way callers don't have to worry about the exact format of the path (should it have a trailing slash, should it be absolute or relative, etc) and can just pass something they know is safe by its type.
@@ -192,11 +181,25 @@ func NewEventBus(logger hclog.Logger) (*EventBus, error) { | |||
logger = hclog.Default().Named("events") | |||
} | |||
|
|||
sourceUrl, err := url.Parse("vault://" + localNodeID) |
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.
Probably worth a changelog note?
vault/eventbus/filter.go
Outdated
func (f *Filters) anyMatch(nsPath string, eventType logical.EventType) bool { | ||
f.lock.RLock() | ||
defer f.lock.RUnlock() | ||
if f.parallel { |
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.
Given that the number of parallel goroutines is equal to the number of nodes (always going to be small), and all the data is local, are we sure it's worth adding this parallel path? I'd err on the side of simplicity until we have data to back up the need for parallelism here.
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. I went back and forth on this one. There are a lot of potential optimizations that could be done (combining/simplifying globs into regexes, parallelization, using a more sophisticated tree matching data structure, etc.), but it all seems a bit premature. I'll remove this for now.
} | ||
filters.patterns = slices.DeleteFunc(filters.patterns, func(m pattern) bool { | ||
return m.eventTypePattern == check.eventTypePattern && | ||
slices.Equal(m.namespacePatterns, check.namespacePatterns) |
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.
Given we're checking for slice equality to remove patterns, perhaps we should sort this slice on the way in to make the equality check a little more reliable?
Co-authored-by: Tom Proctor <[email protected]>
Thanks! |
This adds a very basic implementation of a list of namespace+eventType combinations that each node is interested in by just running the glob operations in for-loops. Some parallelization is possible, but not enabled by default.
It only wires up keeping track of what the local event bus is interested in for now (but doesn't use it yet to filter messages).