fix: add acquire timeout to sqlite database connection #1590
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
This should fix #1503 I wasn't able to trigger enough IO pressure for the SQL connection to be a problem. This adds `local_timeout` to the client config. This is a float, and represents the number of seconds (units in line with the other timeouts, though those are ints). Users may well want to reduce this if they regularly have issues, but by default I think 2s is fine and avoids a non-responsive system in bad situations.
|
Oh also I am still planning on adding an Atuin daemon, which should totally avoid the issue referenced above. Just
|
|
if you have a moment to make a new release, I'd be happy to update the version on nixos and make another test. |
|
Thanks a lot @ellie, it seems great! I'll wait for a release, but no stress about this ofc. My use case is the same as happysalada, I'm using NixOS too, and its rebuilds are very IO intensive. |
jaxvanyang
added a commit
to jaxvanyang/atuin
that referenced
this pull request
Aug 2, 2024
This make it possible to control the timeout of SQLite operations in test. And ATUIN_TEST_LOCAL_TIMEOUT defaults to the default local_timeout, which is actually used in the client. Instead of a small timeout (0.1), this change makes the test less likely to fail and better imitate the default behavior. SQLite operation timeout was first introduced from atuinsh#1590, including connection and store timeout. The env ATUIN_TEST_SQLITE_STORE_TIMEOUT which atuinsh#1703 added only specify the store timeout. This commit doesn't deprecate ATUIN_TEST_SQLITE_STORE_TIMEOUT, but control it by setting its default to the new env ATUIN_TEST_LOCAL_TIMEOUT.
jaxvanyang
added a commit
to jaxvanyang/atuin
that referenced
this pull request
Aug 2, 2024
This make it possible to control the timeout of SQLite operations in test. And ATUIN_TEST_LOCAL_TIMEOUT defaults to the default local_timeout, which is actually used in the client. Instead of a small timeout (0.1), this change makes the test less likely to fail and better imitate the default behavior. SQLite operation timeout was first introduced from atuinsh#1590, including connection and store timeout. The env ATUIN_TEST_SQLITE_STORE_TIMEOUT which atuinsh#1703 added only specify the store timeout. This commit doesn't deprecate ATUIN_TEST_SQLITE_STORE_TIMEOUT, but control it by setting its default to the new env ATUIN_TEST_LOCAL_TIMEOUT.
jaxvanyang
added a commit
to jaxvanyang/atuin
that referenced
this pull request
Aug 2, 2024
This make it possible to control the timeout of SQLite operations in test. And ATUIN_TEST_LOCAL_TIMEOUT defaults to the default local_timeout, which is actually used in the client. Instead of a small timeout (0.1), this change makes the test less likely to fail and better imitate the default behavior. SQLite operation timeout was first introduced from atuinsh#1590, including connection and store timeout. The env ATUIN_TEST_SQLITE_STORE_TIMEOUT which added by atuinsh#1703 only specify the store timeout. This commit doesn't deprecate ATUIN_TEST_SQLITE_STORE_TIMEOUT, but control it by setting its default to the new env ATUIN_TEST_LOCAL_TIMEOUT.
2 tasks
ellie
pushed a commit
that referenced
this pull request
Aug 5, 2024
…ite (#2337) * test: add env ATUIN_TEST_LOCAL_TIMEOUT to control test timeout of SQLite This make it possible to control the timeout of SQLite operations in test. And ATUIN_TEST_LOCAL_TIMEOUT defaults to the default local_timeout, which is actually used in the client. Instead of a small timeout (0.1), this change makes the test less likely to fail and better imitate the default behavior. SQLite operation timeout was first introduced from #1590, including connection and store timeout. The env ATUIN_TEST_SQLITE_STORE_TIMEOUT which added by #1703 only specify the store timeout. This commit doesn't deprecate ATUIN_TEST_SQLITE_STORE_TIMEOUT, but control it by setting its default to the new env ATUIN_TEST_LOCAL_TIMEOUT. * test!: replace ATUIN_TEST_SQLITE_STORE_TIMEOUT with ATUIN_TEST_LOCAL_TIMEOUT This deprecate ATUIN_TEST_SQLITE_STORE_TIMEOUT for simplicity as the new env ATUIN_TEST_LOCAL_TIMEOUT can control both connection and store timeout of SQLite in test. Details see 4d88611. Revert: #1703.
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.
This should fix #1503
I wasn't able to trigger enough IO pressure for the SQL connection to be a problem.
This adds
local_timeoutto the client config. This is a float, and represents the number of seconds (units in line with the other timeouts, though those are ints). Users may well want to reduce this if they regularly have issues, but by default I think 2s is fine and avoids a non-responsive system in bad situations.