Github user davisp commented on the pull request:
https://github.com/apache/couchdb-couch-replicator/pull/38#issuecomment-221400655
Two major thoughts occur to me reading this. First, for responding to 429
responses it seems like something a bit more reactive would be more useful to
users rather than forcing them to figure out limits and set the values
collective. Ie, something in the HTTP retry logic that gets reported to the
rate limiter would be nice. This would hopefully also account for all of the
scenarios where a user has multiple replications that share a source and/or
target. (Or else the user would have to count the replications and then
configure settings appropriately).
Second, I agree with @sagelywizard's comment that we should avoid writing
our own supervisors as much as possible. I'd suggest adding a supervisor with
dynamic children like we already do for the replication jobs themselves. Also,
given that you already have a process per replication with the reset process
I'd get rid of the ets table and reset logic. Just have that process track the
rate limits and handle when it would return a response. That way you don't have
to get into fancy math on multiple requests waiting independently (if that
makes sense).
Much more minor though, the ets table keyed off of the source or target
seems a bit limiting and plausibly not good. You might want to provide some
method for configuring the scope of the rate limit for a given replication.
(ie, source & target, source or target, optionally including user/pass (all
hashed)) as users might only want to rate limit one side of a replication and
or both sides.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
