-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
if err := cm.decayer.Close(); err != nil { | ||
return err | ||
} |
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 think we should still cancel the background goroutine, even if this fails.
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.
Makes sense.
// 1. Manages decay. | ||
// 2. Applies score bumps. | ||
// 3. Yields when closed. | ||
func (d *decayer) process() { |
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.
A performance consideration here is that this loop might run way too often if the resolution is small.
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.
True, maybe we should introduce a hard lower limit for the resolution?
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, that would work.
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, modulo the cancellation of background goroutine in case of errors in Close.
We now allow the user to request tags at an interval lower than the decayer's resolution, and we override the effective interval to be the resolution. We also allow tags to have any interval, not necessarily a multiple of the resolution any longer.
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.
Awesome, thanks @raulk! This looks good to me :)
Depends on libp2p/go-libp2p-core#104. See that PR for an explanation.
Tests use mocked time. We need to use mocked time more often.
Fixes #47.