-
Notifications
You must be signed in to change notification settings - Fork 664
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 Throttled Handler implementation to SDK #1906
Conversation
769f41e
to
7eefc43
Compare
aadb94b
to
3dc9d81
Compare
Co-authored-by: Ceyhun Onur <ceyhun.onur@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
network/p2p/throttled_handler.go
Outdated
|
||
func (t ThrottledHandler) AppGossip(ctx context.Context, nodeID ids.NodeID, gossipBytes []byte) error { | ||
if !t.Throttler.Handle(nodeID) { | ||
return fmt.Errorf("dropping message from %s: %w", nodeID, ErrThrottled) |
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.
q: should we really return an error when throttling? How about logging a warn and return nil. Not sure how AppGossip errors are handled by consensus engine
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.
This is dropped and logged by the p2p.responder
that wraps all p2p.Handler
instances. Currently we log, but in the future we'll implement application-level errors in #1889 where we'll make a custom p2p.AppError
type that the sdk will interpret as an error
.
Not sure how AppGossip errors are handled by consensus engine
Any AppHandler
error will fatal consensus which is why they are logged, learned this when making the SDK. 😓
4c3a490
to
76885a1
Compare
This reverts commit 8d9bf22.
85fb3d2
to
4a1cd30
Compare
Why this should be merged
Adds a throttled app handler implementation to the SDK
How this works
All incoming requests need to clear a throttler
How this was tested
Added unit test