-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add a new event type to notify nodes about dropped inputs #533
base: main
Are you sure you want to change the base?
Conversation
This gives nodes a way to check whether some inputs have been dropped by dora. The event also includes a drop reason, which is currently only the queue size.
I feel like this is a bit of a weird features, as if a node start dropping input is probably because of being under heavy load. I would probably track dropped input at a daemon level but not send additional input to a node that is already under load. |
Could we before adding additional features such as additional event, focus on our current codebase to push stability first? |
My motivation is that it makes it easier to debug issues such as #515. There is currently no way for nodes to know whether inputs were dropped intentionally or because of a bug in dora. So I think that it is related to stability given that users no longer need to look through our GitHub issue tracker to find out why dora dropped some inputs. |
Alternative idea: Instead of adding a new event type, we could add a |
I understand why you would want to add this for debuggability, but I would want to move maybe on other things before handling this error. Can we maybe post pone this PR? Thanks a lot! |
Instead of adding a new ˋDroppedInputsˋ event.
Allows us to add more fields in the future. This is a breaking change.
I pushed some new commits that implement this approach. It's a much smaller change this way.
Sure, this PR is not urgent. I just think that something like this would make dora's behavior clearer for users. |
This gives nodes a way to check whether some inputs have been dropped by dora. The event also includes a drop reason, which is currently only the queue size.
This might make it easier to debug issues such as #515