You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "jasonk000 (via GitHub)" <gi...@apache.org> on 2025/03/03 22:03:45 UTC
[PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
jasonk000 opened a new pull request, #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "jasonk000 (via GitHub)" <gi...@apache.org>.
jasonk000 commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-3271443372
@tolbertam I want to flag that we found an issue with this limiter implementation since you are considering release soon. The scenario is the logger throwing unanticipated exceptions at a time which messes up the state.
Secondly, we found an issue that hampered the investigation -- since we are pushing more load now, the recursion in the request-done-and-submit-next flow is leading to very deep stacks, so we will be switching that to a loop to keep the stack manageable.
Draft is available but we have not yet tested it in real workload -> https://github.com/apache/cassandra-java-driver/compare/4.x...jasonk000:cassandra-java-driver:jkoch/throttler-depth?expand=1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam merged PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "jasonk000 (via GitHub)" <gi...@apache.org>.
jasonk000 commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-2790404505
@adutra, @tolbertam, I've updated the docs and squashed to a single commit. Do let me know if there's anything left to-do!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "jasonk000 (via GitHub)" <gi...@apache.org>.
jasonk000 commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-2730239314
Thanks, @tolbertam, I've squashed it now. Ready to go!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-2730433225
> 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "jasonk000 (via GitHub)" <gi...@apache.org>.
jasonk000 commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-2730414583
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-2696103734
nice @jasonk000! Looking forward to reviewing this, will try to give it a look this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-3275103789
thanks for the heads up @jasonk000 (FYI @absurdfarce)
> The scenario is the logger throwing unanticipated exceptions (when we are nearing OOM) at a time which messes up the counter state. We can fix this by tracking and unwinding at the right moment and ensuring correct operation ordering before calling the logger.
Oh that sounds fun, OOMs causing runtime exceptions to be thrown in unexpected places can definitely screw things up in unpredictable ways. I suppose since this PR added some logging that might explain why hadn't seen this before? Although I do see there was logging previously in some places.
> Secondly, we found an issue that hampered the investigation -- since we are pushing more load now, the recursion in the request-done-and-submit-next flow is leading to very deep stacks, so we will be switching that to a loop to keep the stack manageable.
Is that a new issue or something that would have previously existed prior to this PR? Has been a while since I looked at this PR so would need to context set, but don't recall this changing how things are called, so guessing that would have been a previous problem? Or is it something subtle in the queuing changes that contribute?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "jasonk000 (via GitHub)" <gi...@apache.org>.
jasonk000 commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-3275932012
> I suppose since this PR added some logging that might explain why hadn't seen this before? Although I do see there was logging previously in some places.
Yeah, good question. The existing released code in 4.19.0 DOES contain a similar issue, however it is in a different piece of the code that is less likely to be triggered.
In the old code we have [one](https://github.com/apache/cassandra-java-driver/blob/4.19.0/core/src/main/java/com/datastax/oss/driver/internal/core/session/throttling/ConcurrencyLimitingRequestThrottler.java#L195) spot that can trigger it.
If the logger throws here, then the `concurrentRequests` would never be decremented. If this happens enough, then you will eventually consume all slots.
In the new code we have [one](https://github.com/jasonk000/cassandra-java-driver/blob/fda2d0eb171a47c9861a8f44c32af27ba450e4fb/core/src/main/java/com/datastax/oss/driver/internal/core/session/throttling/ConcurrencyLimitingRequestThrottler.java#L99-L102) [two](https://github.com/jasonk000/cassandra-java-driver/blob/fda2d0eb171a47c9861a8f44c32af27ba450e4fb/core/src/main/java/com/datastax/oss/driver/internal/core/session/throttling/ConcurrencyLimitingRequestThrottler.java#L113-L114) [three](https://github.com/jasonk000/cassandra-java-driver/blob/fda2d0eb171a47c9861a8f44c32af27ba450e4fb/core/src/main/java/com/datastax/oss/driver/internal/core/session/throttling/ConcurrencyLimitingRequestThrottler.java#L127-L128). You can see they are more directly in the line of fire and much easier to trigger.
You can see [in this commit to fix it](https://github.com/apache/cassandra-java-driver/commit/b3cdb43de6a791e76f13a0737561d11ce2e068a5) that we only have to enforce it in a few specific areas.
----
> .. recursion ..
> Is that a new issue or something that would have previously existed prior to this PR
You are correct, this is a new issue/fix and can probably be considered separate. Maybe I should put together a separate PR just for the smaller logger handler changes and deal with the loop/recursion one separate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on code in PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#discussion_r1997802648
##########
core/src/main/java/com/datastax/oss/driver/internal/core/session/throttling/ConcurrencyLimitingRequestThrottler.java:
##########
@@ -61,17 +60,12 @@ public class ConcurrencyLimitingRequestThrottler implements RequestThrottler {
private final String logPrefix;
private final int maxConcurrentRequests;
private final int maxQueueSize;
-
- private final ReentrantLock lock = new ReentrantLock();
-
- @GuardedBy("lock")
- private int concurrentRequests;
-
- @GuardedBy("lock")
- private final Deque<Throttled> queue = new ArrayDeque<>();
-
- @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
Review Comment:
thanks, notable comment as would have questioned this otherwise 😛
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "jasonk000 (via GitHub)" <gi...@apache.org>.
jasonk000 commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-3140930180
Hi @tolbertam, putting this on the radar as well - hoping it can make it for 4.19.1. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-3141114462
thanks for bringing this to my attention @jasonk000 , i'll amend the commit to add the Patch; Reviewed by section and merge this shortly!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "tolbertam (via GitHub)" <gi...@apache.org>.
tolbertam commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-3141123312
Done, thank you again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] Eliminate lock in ConcurrencyLimitingRequestThrottler [cassandra-java-driver]
Posted by "akhaku (via GitHub)" <gi...@apache.org>.
akhaku commented on PR #2025:
URL: https://github.com/apache/cassandra-java-driver/pull/2025#issuecomment-2695711912
reviewed internally, LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org