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

feat: implement terminate_pool/2 callback #38

Merged
merged 4 commits into from
Nov 11, 2023
Merged

Conversation

oliveigah
Copy link
Contributor

As mentioned on #37, in order to implement pool telemetry features on nimble_pool clients would be nice to have some callback regarding pool termination.

The implementation is very straight forward but I'm not sure if the changes that I made to the tests are the best approach.

I had to always accept terminate_pool as a valid instruction.

The stateless is tricky, because this callback does not have access to worker state and therefore I need to work around this by introspecting into pool_state it self and if more complex use cases must be tested we gonna need another work around to update the remaining instructions.

The stateful implementation I think is ok, because terminate_pool is valid only when there is no more instructions and also there is no need for a workaround on the worker state.

Let me know what you think! Thanks in advance! 😃

@oliveigah oliveigah changed the title feat: implement terminal_pool/2 callback feat: implement terminate_pool/2 callback Nov 5, 2023
lib/nimble_pool.ex Outdated Show resolved Hide resolved
@josevalim
Copy link
Member

For the tests, you can probably create a separate pool module or a separate worker. Just start the pool with a single worker, and then message the parent. Verify that terminate_worker is called before terminate_pool and that's it. :)

oliveigah and others added 2 commits November 11, 2023 14:36
@oliveigah
Copy link
Contributor Author

For the tests, you can probably create a separate pool module or a separate worker. Just start the pool with a single worker, and then message the parent. Verify that terminate_worker is called before terminate_pool and that's it. :)

Great! So I think the stateful pool test is enough! I removed the terminate_pool callback from the stateless one since it may hide some bugs due to the test implementation.

Let me know what you think!

@josevalim josevalim merged commit a0aa89d into dashbitco:main Nov 11, 2023
3 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

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