timokau
commented
3 years ago Yes, I agree. For reference, here is my current view on rate limiting: https://github.com/timokau/marvin-mk2/issues/34#issuecomment-714386315
it might make sense to instead count the messages a person sent in the last N days (plus the tags marvin explicitly sent / reviews explicitly requested by PR authors, maybe).
That would require state. Until now I tried to avoid that. That's not a deal-breaker, but it would complicate things. So I think the "max-in-flight" (number of assigned PRs that have been updated in the last n days, possibly ignoring updates after merge) approach might work better.
Based on the comments on https://github.com/timokau/marvin-mk2/pull/101, it sounds like currently marvin's rate-limiting algorithm is based on “involves the person and was updated in the last N days”.
This attempts to limit the amount of notification, and thus makes sense.
However, IMO it would make more sense to instead limit the amount of comments a person is expected to make: we can't count notifications from the discourse anyway, etc. So IMO instead of trying to count the notifications a person received (and necessarily failing at it because github does not say who (un)subscribed), it might make sense to instead count the messages a person sent in the last N days (plus the tags marvin explicitly sent / reviews explicitly requested by PR authors, maybe).
This way, if one is passive in nixpkgs interactions (eg. due to commenting on one or a few high-traffic nixpkgs issues/PRs), they would get new marvin requests for things that are actually actionable.
What do you think?
(Note: this is just a prospective issue, for gathering ideas. IMO the current rate-limiting algorithm is good enough to go in production, and then it'll be possible to iterate on it — assuming that the reviewers list can somehow get pushed to nixpkgs so that people could easily adjust their reviewing settings)