Add timeout support in queries
#47
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lpil
reviewed
Nov 25, 2024
ghivert
force-pushed
the
main
branch
2 times, most recently
from
a847668 to
14654e1
Compare
It's done! |
lpil
reviewed
Dec 5, 2024
test/pog_test.gleam
Outdated
| let db = start_default() | ||
|
|
||
| pog.query("select sub.ret from (select pg_sleep(0.1), 'OK' as ret) as sub") | ||
| |> pog.timeout(1000) |
src/pog.gleam
Outdated
| @@ -54,6 +54,9 @@ pub type Config { | |||
| /// (default: False) By default, pgo will return a n-tuple, in the order of the query. | |||
| /// By setting `rows_as_map` to `True`, the result will be `Dict`. | |||
| rows_as_map: Bool, | |||
| /// (default: 5000): Default time to wait before the query is considered timeout. | |||
| /// Timeout can be edited per query. | |||
| } | ||
|
|
||
| /// Push a new query parameter value for the query. | ||
| pub fn parameter(query: Query(t1), parameter: Value) -> Query(t1) { | ||
| Query(..query, parameters: [parameter, ..query.parameters]) | ||
| } | ||
|
|
||
| pub fn timeout(query: Query(t1), timeout: Int) -> Query(t1) { |
|
Done |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi!
Goal
This PR aims to add timeout support to
pog.This PR serves the purpose to start the conversation to find the best API & the best implementation, and is probably subject to future iterations, so take it as is.
To implement timeout, the idea consists to completely overrides the default parameter of
pgo, to rely only onpog.Timeout configuration is now stored in two places: the default configuration, and per-query. This makes sense if you consider some individual queries have to run longer than others, maybe because they won't be exposed on the WWW, but used asynchronously in actors. In the same time, having the default configuration allows for a centralized way to make sure every queries have a correct default timeout value.
By default, timeout has a value of 5000 ms, the current value provided by
pgo.Implementation details
To pass options to
pgo:query, we need to target the 3rd argument ofquery/3. As of now, it's only used to pass the pool atom. The PR modifiesrun_queryin Gleam to support a 4th argument, here the timeout.I hesitated to directly pass the
pool_optionstuple needed inpgo, or simply the timeout like it is now. Passing the whole options record would allow for easier increments in the future, to add new features. In the same time, simply passingOption(Int)makes a really simple API, and it could be iterated later.I'd be happy to discuss about it, and I'd be happy to change the way it works now to implement better options handling. They should probably be more thought out to find the correct format?
Currently,
pgodoes not properly handle timeout, and simply returns the{error, closed}tuple when the query times out. The issue should be tracked in erleans/pgo#96.EUnit defaults needed to be updated to avoid timeouts after 5s. By default, EUnit timeout after 5s. To properly test timeouts, I need to increase the timeouts to test them. The time could probably be modified though.
Issues