feat: skip sending pending payload if URL doesn't match #5834
Conversation
|
This is technically ready for review, but working through this led me to some thoughts about a potential refactor of this overall pending request behavior. @obycode would love your thoughts on this! |
| @@ -438,6 +438,10 @@ impl EventObserver { | |||
| }; | |||
|
|
|||
| for (id, url, payload, timeout_ms) in pending_payloads { | |||
| // If the URL is not the same as the endpoint, skip it | |||
| if !url.starts_with(&self.endpoint) { | |||
There was a problem hiding this comment.
Is starts_with sufficient here? We should probably be more explicit so that it handles a case where the DB contains the URL "http://localhost:8000/foo" and this EventObserver has the endpoint "http://localhost:80".
There was a problem hiding this comment.
I like your idea of refactoring so that it is moved into the EventDispatcher structure. The biggest problem I have with this mechanism is that when there is an old endpoint in the DB, each observer will check it every time they send a payload, adding unnecessary overhead.
This PR updates the event dispatcher functionality to not send pending requests if the URL is no longer registered as an event observer.
The way pending requests are implemented is that each individual event dispatcher instance checks for pending requests before it sends any new ones. If a node has multiple event observers, this code is ran for each one of them. The first dispatcher is the only one that will actually handle pending requests, if there are any.
Now, the code is updated so that each dispatcher gets pending requests, but then it compares that request to its own URL. If the URL doesn't match, the pending request is skipped.
I think this implementation leaves something to be desired, although I think the ideal change would involve a small refactor of this code. Currently, we're performing IO (O(n)) for each dispatcher for each request, even though we should only ever have pending requests on bootup, and then we should never check again. This would probably be best located in the
EventDispatcherstruct (of which we only have one) instead ofEventObserver.