Allow esqlite’s timeout to be specified #65
Conversation
|
Hello, @marramgrass! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
lib/sqlitex/server.ex
Outdated
| {:ok, result, new_cache} -> {:reply, {:ok, result}, {db, new_cache}} | ||
| err -> {:reply, err, {db, stmt_cache}} | ||
| def handle_call({:query, sql, opts}, _from, {db, stmt_cache, timeout}) do | ||
| case query_impl(sql, opts,stmt_cache, timeout) do |
marramgrass
force-pushed
the
esqlite-timeouts
branch
from
ce09ec4 to
3f4f73e
Compare
lib/sqlitex.ex
Outdated
| @spec close(connection) :: :ok | ||
| def close(db) do | ||
| :esqlite3.close(db) | ||
| @esqlite3_timeout Application.get_env(:sqlitex, :esqlite3_timeout, 5_000) |
There was a problem hiding this comment.
I could be wrong, but doesn't storing this in a module attribute mean that this gets read from the app config at compile time? So if I update the :esqlite3_timeout in my config but don't explicitly recompile sqlitex then that change won't be picked up?
I guess maybe Elixir is smart enough to pick up that the change in sqlitex config might require a sqlitex recompile, but I'm not sure if that is the case...
There was a problem hiding this comment.
You know, I'm not sure either.
If we went with changing these to use keyword lists anyway then the intended benefit of using module attributes (easy specification of default values in function definitions) is no longer needed.
Thanks 👍
lib/sqlitex.ex
Outdated
| def open(path) do | ||
| :esqlite3.open(path) | ||
| @spec open(charlist | String.t, integer()) :: {:ok, connection} | {:error, {atom, charlist}} | ||
| def open(path, timeout \\ @esqlite3_timeout) |
There was a problem hiding this comment.
I'm not sure, but would it be better to expose these parameters as keyword list? That way if we need to add other options we can just add new keywords rather than needing to add a whole new parameter...
There was a problem hiding this comment.
I agree, I think it would be preferable. Will update accordingly.
|
@marramgrass thank you for this PR. I think that allowing the timeout to be passed through is a great idea and doing it with optional keyword options, then an |
|
For info, I will be coming back to this to update. Just very busy with other things at the moment. Sorry for the delay. |
|
@marramgrass thanks for letting us know. I'm happy to wait for you to make the changes, or if you'd like some help I can take some time to make those changes myself. |
lib/sqlitex.ex
Outdated
| :esqlite3.open(path) | ||
| @spec open(charlist | String.t, Keyword.t) :: {:ok, connection} | {:error, {atom, charlist}} | ||
| def open(path, opts \\ []) | ||
| def open(path,opts) when is_binary(path), do: open(string_to_charlist(path), opts) |
|
I've pushed a further commit making the discussed changes. Please let me know if you'd rather I squashed them down to one. |
Also changed to use :db_timeout as the key in most places, for brevity. Retained :esqlite3_timeout in the Application env.
marramgrass
force-pushed
the
esqlite-timeouts
branch
from
376ef9d to
af7a286
Compare
|
Thanks @marramgrass for this contribution. I think I'll change the name of the global config to match the |
|
Got 1.4.0 released with your changes included. Thank you again! |
Hello.
First, thank you for the library. It's come in very useful for us.
We've been doing some work where we needed to increase the timeout
esqliteapplies to various operations. This pull request updates Sqlitex to expose the timeout argument in the various places thatesqlitedoes so.With these changes, Sqlitex defaults to the
esqlitetimeout of 5000 ms, and uses a combination of default values on the function arguments and implementing anApplication-level config to leave the interface of Sqlitex unchanged for those who don't need to modify the timeout.I think this is a broadly useful enough change to consider bringing into the upstream, but I worry that the implementation is a little noisy. Please let me know any changes you want made.
Thanks again.