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

new pool callback for idle verification #46

Closed
oliveigah opened this issue Nov 1, 2024 · 4 comments
Closed

new pool callback for idle verification #46

oliveigah opened this issue Nov 1, 2024 · 4 comments

Comments

@oliveigah
Copy link
Contributor

In Finch, we have a bug report regarding idle termination: sneako/finch#291

To address this, I added the last checkout timestamp to the pool state and verified it using the currently available handle_ping/2 callback for workers: sneako/finch#292

While handle_ping/2 works well for managing individual idle resources, it’s less suited for making pool-wide decisions, as it lacks full context of the pool’s idle state.

For example, consider a pool with: resource 1 (idle), resource 2 (just checked in, not idle)

If idle verification triggers handle_ping/2 for resource 1, there’s no way to confirm if the entire pool is idle and should be terminated.

Since this seems to be a more general problem, I propose adding a handle_ping/1 callback for the pool itself, which would trigger after a given timeout without checkouts or check-ins and therefore provides better semantics for pool termination scenarios.

This would also prevent clients like Finch from needing to track internal metadata for the pool, such as the last checkout timestamp.

Let me know your thoughts! If you agree, I can start to work on a PR.

@josevalim
Copy link
Member

Are we sure adding a handle_ping/1 would be better? Now NimblePool would need to check when all resources have not been used, requiring the pool to track more additional state. Between tracking the state of all resources and a timestamp, it feels the timestamp is a more lightweight approach?

@oliveigah
Copy link
Contributor Author

I was thinking about a more generic approach, where handle_ping/1 would only trigger after the pool has been idle—no checkouts or check-ins—for more than X time.

With this approach, handle_ping/1 would provide exactly the semantics we need: we could terminate the pool as soon as it’s called, with no extra data or checks required.

The difference is in the semantics:

handle_ping/2: triggers for each worker when it has been idle for more than X time.

handle_ping/1: triggers for the entire pool when it has been idle (no checkouts or check-ins) for more than X time.

The handle_ping/1 semantics seem far more suited to pool termination scenarios, and they would simplify cases where we need to terminate the pool due to inactivity.

@josevalim
Copy link
Member

@oliveigah since my concern is the complexity of implementation, would you like to send a PR? My gut feeling still thinks that handle_ping/2 + last_checkout_timeout is enough, but perhaps the implementation here is so simple that it won't really matter.

@oliveigah
Copy link
Contributor Author

Sure! Gonna take a look at it!

Regarding implementation I think it would be simple and similar to what I've done on finch.

I was thinking about just have an extra field on the state last_activity_ts and compare on it inside a periodic check to see if we shoul call handle_ping/1 callback or not.

@josevalim josevalim closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2024
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 a pull request may close this issue.

2 participants