-
Notifications
You must be signed in to change notification settings - Fork 19
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
Introduce filter_predicate
to tame event bus traffic.
#34
Introduce filter_predicate
to tame event bus traffic.
#34
Conversation
fe6bf88
to
d61c088
Compare
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 suspect you're still working on this but just in case it isn't on the TODO list, a code example in the docs showing how to use this new API would be valuable.
lahja/endpoint.py
Outdated
|
||
if config is None or config.filter_endpoint is None: | ||
# Check if a `filter_predicate` applies | ||
if listening_endpoint.config.filter_predicate is None: |
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.
Up-front I want to say i'm not sure this suggestion is actually better but I want to toss it out and see if it sticks.
This value checking seems.... ?unfortunate?. Lots of logic branches that might be reducable if the None
version of at least one of these was able to be captured via the same API as when it is present. I.E. maybe there should always be a filter_predicate
that unconditionally returns True
.
Similarly with the config
, could there be a default empty config which gets attached by default which exhibits the same behavior as if it was left out?
This article is topical to this. https://blog.conjur.org/special-cases-are-a-code-smell/
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 agree. In fact, a lot of this is based on a misconception that I had around Pythons default class attribute handling that scared me. I thought that the following was a footgun in Python
class Foo:
y = ''
Similar to
class Foo:
y = []
But while the latter is indeed a footgun, the former isn't because strings are immutable in Python. Based on this wrong assumption I used a lot of Optiona[str]
what could have been just str
with strings preassigned to ''
.
Will clean it up.
d61c088
to
7e3c90d
Compare
@pipermerriam This is now ready for review. Notice that I also renamed all the I find Trinity PR that uses this will follow shortly. |
I spent some time skimming over this and I wonder if there's a different architecture which might work better. It took me a while to understand what was happening: I think this design has a few drawbacks:
Since this is a performance optimization, maybe the interface doesn't have to change at all! The Endpoint knows exactly which |
Thank you for the review Brian. Let me dig in!
Wait. But this is what it does no? producer.add_listener_endpoints(
ListenerConfig.from_name(
'receiver',
filter_predicate=lambda ev: not isinstance(ev, BadEvent),
)
) What happens here is that the
In Trinity, every ( Here's an example that I'm currently using to work on the sync. @staticmethod
def should_receive_broadcast(event: BaseEvent) -> bool:
return any((
isinstance(event, PeerJoinedEvent),
isinstance(event, PeerLeftEvent),
isinstance(event, PeerPoolMessageEvent) and isinstance(event.cmd, NewBlock),
BaseIsolatedPlugin.should_receive_broadcast(event),
)) It is true that the listener needs to come up with the
Yes, that is true but I would argue that these things can happen on a micro level using the
While I like the idea in general I'm hesitant because I think it is much harder and might be a case of YAGNI. Here are some thoughts:
I'm not saying this is something that can't be solved but given the fact that one is free how to setup the topology, automatically broadcasting event subscriptions between the right endpoints may not be as simple as it looks on first sight.
The Not sure how we'd solve that other than redesigning the events so that every command has it's entirely own event type. But even then, the So, basically what I'm saying is, maybe the |
So dropping this in favor of #55 right? |
Yes, will close this. |
What was wrong?
Obviously, the
Endpoint
provides APIs to subscribe to only specific events (subscribe
,stream
,wait_for
,request
). However, using these APIs, we do only filter events within our currentEndpoint
to only receive specific events at specific callsites.But that doesn't mean these are the only events that are pushed into our
Endpoint
. Which events are pushed into which endpoints is controlled by theEndpoint
that is broadcasting the event (Assuming we are listening to thatEndpoint
at all).The logic for that is rather simpe so far:
If an
event
is only intended for a specificEndpoint
, only push it into thatEndpoint
Otherwise, push it into every listening
Endpoint
.The producer of these events that are broadcasted over all listening endpoints does and should not care about who needs which events. However, this can become a bottleneck.
Let's use Trinity as an example. Once we start broadcasting all peer messages from the
PeerPool
across the listening endpoints, pushing every new block, receipt, transaction etc. into every listeningEndpoint
is very wasteful.We may be tempted to say, "well, if a process isn't interested in these events, why listen to the
PeerPool
events at all"? Because, we may be interested in some of the events. E.g. theEthStats
plugin periodically wants to now the current peer count but it does not need all the constant chatter of connected peers.To put out some numbers, imagine sending something to
10
processes.When actually only one of them needs it:
How was it fixed?
With this PR, when we start listening to another
Endpoint
, we can provide afilter_predicate
. It's a bit likesubscribe
orstream
but on a macro level. It let's us shield from any events that we do not want to even reach ourEndpoint
because we know that no callsite is interested in it at all.The
filter_predicate
is aCallable[[BaseEvent], bool]
that is expected to returnTrue
to let an event go through or returnFalse
to shield from an event.The exact rules are as follows:
filter_predicate
-> not shielding from anythingBroadcastConfig
-> not shieldedEndpoints
-> only send to us iffilter_predicate(event)
returnsTrue
.Cute Animal Picture