-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Refactor (Process): DB writer #8352
base: develop
Are you sure you want to change the base?
Conversation
92f0da7
to
a9f73ca
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8352 /- ##
===========================================
- Coverage 59.97% 57.75% -2.23%
===========================================
Files 1755 1755
Lines 124891 124809 -82
Branches 16729 16279 -450
===========================================
- Hits 74904 72083 -2821
- Misses 47539 50249 2710
- Partials 2448 2477 29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
0850053
to
6bb7716
Compare
355ea97
to
0ee5557
Compare
0ee5557
to
0a9467e
Compare
rotkehlchen/api/rest.py
Outdated
write_cursor.execute('DELETE FROM zksynclite_transactions WHERE tx_hash=?;', (tx_hash,)) # noqa: E501 | ||
concerning_address = cursor.execute( | ||
'SELECT from_address FROM zksynclite_transactions WHERE rowid=?', | ||
(write_cursor.lastrowid,), | ||
).fetchone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: here you are using that the writter cursor hasn't commited yet to query for the row id right? I see that this can be error prone if at some point we change the logic. Why not write it like
write_cursor.execute('DELETE FROM zksynclite_transactions WHERE tx_hash=?;', (tx_hash,)) # noqa: E501 | |
concerning_address = cursor.execute( | |
'SELECT from_address FROM zksynclite_transactions WHERE rowid=?', | |
(write_cursor.lastrowid,), | |
).fetchone() | |
concerning_address = cursor.execute( | |
'SELECT from_address FROM zksynclite_transactions WHERE tx_hash=?', | |
(tx_hash,), | |
).fetchone() | |
write_cursor.execute('DELETE FROM zksynclite_transactions WHERE tx_hash=?;', (tx_hash,)) # noqa: E501 |
rotkehlchen/api/rest.py
Outdated
deleted_event_data = cursor.execute( | ||
'SELECT location_label FROM history_events WHERE rowid=?', | ||
(write_cursor.lastrowid,), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above, why not interchange the order of the queries? I believe is better for readibility and also can avoid errors in the future
with data_updater.user_db.conn.write_ctx() as write_cursor: | ||
write_cursor.execute( | ||
'INSERT INTO rpc_nodes(name, endpoint, owned, active, weight, blockchain) ' | ||
'VALUES(?, ?, ?, ?, ?, ?)', | ||
custom_node_tuple, | ||
) | ||
cursor.execute('SELECT COUNT(*) FROM rpc_nodes') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to close the cursor before reading to ensure that the other connection can see it right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with database.user_write() as cursor: | ||
with database.conn.read_ctx() as cursor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it better in cases like this test to open the read/write cursor that you have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reader can be used like that, but writer has to commit changes
106e6bc
to
906423c
Compare
concerning_address = cursor.execute( | ||
'SELECT from_address FROM zksynclite_transactions WHERE tx_hash=?', | ||
(tx_hash,), | ||
).fetchone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this introduces a bug since you have a tuple and not an address
rotkehlchen/start.py
Outdated
for process_type, process_data in manager.processes.items(): | ||
process_data.process = Popen(executable ['--process', process_type.value]) # noqa: S603 | ||
|
||
if (backend_process := manager.processes[ProcessType.API_SERVER].process) is not None: | ||
backend_process.wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that I noticed is that you don't check that all the processes are running right? It can happen that the db writter fails to start and you don't check it
906423c
to
cccb7f2
Compare
cccb7f2
to
0eae0d3
Compare
Closes #7272
Checklist