wip: remove connection pool from sqlite #8797
Conversation
otherwise, we create consumers, which does not work well with windows.
abyesilyurt
force-pushed
the
aziz/sqlite_no_pool
branch
from
5508ea0
to
0099332
Compare
| @@ -76,7 +76,7 @@ def compute() -> int: | |||
|
|
|||
| client_low_ds.refresh() | |||
| res = client_low_ds.code.compute(blocking=True) | |||
| assert res == compute(blocking=True).get() | |||
| assert res == compute(syft_no_node=True) | |||
There was a problem hiding this comment.
We start threads when syft_no_node=False, which leads to unstable tests.
| return Err( | ||
| f"Failed to acquire lock for the operation {self.lock.lock_name} ({self.lock._lock})" | ||
| ) | ||
| # locked = self.lock.acquire(blocking=True) |
There was a problem hiding this comment.
We don't need locks for sqlite anymore. Thread safety is done by sqlite on file level.
Mongodb should handle thread safety as well.
| @@ -22,6 +23,31 @@ def test_sqlite_store_partition_sanity( | |||
| assert hasattr(sqlite_store_partition, "searchable_keys") | |||
|
|
|||
|
|
|||
| def test_sqlite_store_partition_benchmark( | |||
There was a problem hiding this comment.
test to benchmark performance. to be deleted after reviews.
There was a problem hiding this comment.
Could def _cursor just yield con.cursor() instead of
cur = con.cursor()
yield cur.execute(sql, *args)?
We can rewrite this
with self._cursor(
insert_sql, [str(key), _repr_debug_(value), data, str(key)]
) as _:
passinto
with self._cursor as cursor:
cursor.execute(insert_sql, [str(key), _repr_debug_(value), data, str(key)])looks a bit more idiomatic to me.
I agree that it looks better, but I think this change is out of scope for this PR as it leads to much bigger refactoring. I will create an issue for this one though. |
Sqlite connection is not thread-safe. We use complicated and unstable pooling to make connections thread-safe. We don't need to do any locking if we just open the connection per each thread.
The tests are also much more stable now.