Eliminate lock in ConcurrencyLimitingRequestThrottler #2025
Conversation
|
reviewed internally, LGTM |
|
nice @jasonk000! Looking forward to reviewing this, will try to give it a look this week. |
There was a problem hiding this comment.
apologies that this took me a bit to get around to, this look great @jasonk000, 👍
| @GuardedBy("lock") | ||
| private boolean closed; | ||
| private final AtomicInteger concurrentRequests = new AtomicInteger(0); | ||
| // CLQ is not O(1) for size(), as it forces a full iteration of the queue. So, we track |
There was a problem hiding this comment.
thanks, notable comment as would have questioned this otherwise 😛
jasonk000
force-pushed
the
concurrency-limit-throttler-unblock-redux
branch
from
c9b8e35 to
70bc953
Compare
|
Thanks, @tolbertam, I've squashed it now. Ready to go! |
Following from 6d3ba47 this changes the throttler to a complete lock-free implementation. Update the related comments and README now that it is lock-free.
jasonk000
force-pushed
the
concurrency-limit-throttler-unblock-redux
branch
from
70bc953 to
bf694bd
Compare
|
Good catch, I've updated related comment & javadocs. Separately, @adutra, we do not internally use the RateLimitingRequestThrottler. If there are other users, I would be happy to put some code together to do the same change, but would need some support to test and verify. Let me know if that would be useful. |
I'd say, let's wait until there is demand for that, but thanks for the initiative! |
Following up from #1957 this moves to a complete lock-free implementation.
(cc @tolbertam who reviewed the previous PR, and @akhaku and @clohfink who have reviewed internally).
Commits are broken to smaller steps in case it's helpful in review. I will do a squash/rebase when OK.