Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add totalWaiters to PoolStats #3

Closed
wants to merge 1 commit into from

Conversation

steve-chavez
Copy link

@steve-chavez steve-chavez commented Jan 28, 2022

Hello @avanov ,

As mentioned in bos#42 (comment), I'd like to limit the waiter queue to do backpressure. So I've added totalWaiters to PoolStats.

Related: PostgREST/postgrest#2042 (comment)

LMWYT, thanks!

@avanov
Copy link
Owner

avanov commented Feb 3, 2022

This looks good and useful semantically. What I'm not sure of is whether the length of the queue should be calculated here or inside WaiterQueue module. Logically it's a property of the queue entity that could be read from it, but never modified directly.

@steve-chavez
Copy link
Author

@avanov Thanks for the review. I did it like this because I also plan to add maxWaiters and I think that wouldn't fit inside the WaiterQueue module. Seemed more consistent to have them both outside.

@steve-chavez steve-chavez marked this pull request as draft February 8, 2022 14:55
@avanov
Copy link
Owner

avanov commented Feb 8, 2022

@steve-chavez yeah I get that from the linked discussion, though aren't they two different concepts? One has to do with a "peekable" stat value (property of the queue), another has to do with utilising all available underlying stats for higher level interface of back-pressuring (back-pressuring being a property of the queue). For instance, the following sequence:

removeSelf <- push waiters var
modifyTVar_ waitersSizeVar (+ 1)

it assumes that push always leads to (+1) for the stat value, and that pop results in (subtract 1).

@steve-chavez
Copy link
Author

@avanov Sorry for the late reply here. Since the newest hasql-pool doesn't use resource-pool(ref), I no longer need this change(we're transitioning to the newest hasql-pool PostgREST/postgrest#2391).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants