Refactor (Process): DB writer #8352

OjusWiZard · 2024-08-02T17:19:14Z

Checklist

Seperate DB write process from API process
Communication between both the processes
Adjust existing code to support this change

codecov · 2024-08-06T11:40:00Z

Codecov Report

Attention: Patch coverage is 79.15493% with 148 lines in your changes missing coverage. Please review.

Project coverage is 57.75%. Comparing base (55403b6) to head (906423c).
Report is 1 commits behind head on develop.

Files	Patch %	Lines
rotkehlchen/db/drivers/server.py	59.84%	53 Missing ⚠️
rotkehlchen/db/drivers/client.py	90.17%	3 Missing and 8 partials ⚠️
rotkehlchen/start.py	84.00%	5 Missing and 3 partials ⚠️
rotkehlchen/db/eth2.py	73.33%	2 Missing and 2 partials ⚠️
rotkehlchen/db/evmtx.py	87.50%	3 Missing and 1 partial ⚠️
rotkehlchen/db/dbhandler.py	97.11%	1 Missing and 2 partials ⚠️
frontend/app/src/subprocess-handler.ts	0.00%	2 Missing ⚠️
rotkehlchen/chain/zksync_lite/manager.py	86.66%	1 Missing and 1 partial ⚠️
rotkehlchen/exchanges/bitstamp.py	33.33%	2 Missing ⚠️
rotkehlchen/api/rest.py	88.88%	1 Missing ⚠️
... and 58 more

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

Flag	Coverage Δ
backend	`80.85% <79.37%> (-0.21%)`	⬇️
frontend_integration	`56.77% <ø> ( 0.01%)`	⬆️
frontend_unit	`31.09% <0.00%> (-5.29%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

frontend/app/src/subprocess-handler.ts

rotkehlchen/start.py

yabirgb · 2024-08-13T16:22:47Z

rotkehlchen/api/rest.py

+ 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()


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

Suggested change

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

yabirgb · 2024-08-13T16:23:54Z

rotkehlchen/api/rest.py

+ deleted_event_data = cursor.execute(
+ 'SELECT location_label FROM history_events WHERE rowid=?',
+ (write_cursor.lastrowid,),


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

rotkehlchen/balances/manual.py

rotkehlchen/chain/evm/decoding/hop/decoder.py

rotkehlchen/tests/db/test_ranges.py

yabirgb · 2024-08-14T14:29:18Z

rotkehlchen/tests/unit/test_data_updates.py

+ 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')


you need to close the cursor before reading to ensure that the other connection can see it right?

yabirgb · 2024-08-14T14:30:02Z

rotkehlchen/tests/unit/test_loopring.py

- with database.user_write() as cursor:
+ with database.conn.read_ctx() as cursor:


isn't it better in cases like this test to open the read/write cursor that you have?

reader can be used like that, but writer has to commit changes

tools/pyinstaller_hooks/hook-zmq.py

rotkehlchen/tests/utils/premium.py

yabirgb · 2024-08-15T16:02:07Z

rotkehlchen/api/rest.py

+ concerning_address = cursor.execute(
+ 'SELECT from_address FROM zksynclite_transactions WHERE tx_hash=?',
+ (tx_hash,),
+ ).fetchone()


I believe this introduces a bug since you have a tuple and not an address

rotkehlchen/db/drivers/server.py

yabirgb · 2024-08-16T09:38:11Z

rotkehlchen/start.py

+ 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()


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

OjusWiZard force-pushed the feat/db_writer_process branch 4 times, most recently from 92f0da7 to a9f73ca Compare August 6, 2024 11:34

yabirgb reviewed Aug 6, 2024

View reviewed changes

frontend/app/src/subprocess-handler.ts Outdated Show resolved Hide resolved

rotkehlchen/start.py Outdated Show resolved Hide resolved

OjusWiZard force-pushed the feat/db_writer_process branch 3 times, most recently from 0850053 to 6bb7716 Compare August 7, 2024 18:21

yabirgb temporarily deployed to macos_sign August 7, 2024 18:24 — with GitHub Actions Inactive

yabirgb temporarily deployed to docker August 7, 2024 18:24 — with GitHub Actions Inactive

yabirgb temporarily deployed to macos_sign August 7, 2024 18:24 — with GitHub Actions Inactive

yabirgb had a problem deploying to windows_sign August 7, 2024 18:24 — with GitHub Actions Failure

OjusWiZard force-pushed the feat/db_writer_process branch 15 times, most recently from 355ea97 to 0ee5557 Compare August 13, 2024 15:11

OjusWiZard changed the title ~~WIP: DB writer process~~ Feat(Process): DB writer Aug 13, 2024

OjusWiZard changed the title ~~Feat(Process): DB writer~~ Refactor (Process): DB writer Aug 13, 2024

OjusWiZard force-pushed the feat/db_writer_process branch from 0ee5557 to 0a9467e Compare August 13, 2024 15:46

OjusWiZard marked this pull request as ready for review August 13, 2024 15:52

OjusWiZard added the ready for peer review Backend PR ready to be reviewed by colleagues label Aug 13, 2024

yabirgb reviewed Aug 14, 2024

View reviewed changes

OjusWiZard force-pushed the feat/db_writer_process branch 3 times, most recently from 106e6bc to 906423c Compare August 15, 2024 15:31

yabirgb reviewed Aug 16, 2024

View reviewed changes

OjusWiZard force-pushed the feat/db_writer_process branch from 906423c to cccb7f2 Compare August 16, 2024 19:24

Refactor (Process): DB writer

0eae0d3

OjusWiZard force-pushed the feat/db_writer_process branch from cccb7f2 to 0eae0d3 Compare August 16, 2024 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor (Process): DB writer #8352

Refactor (Process): DB writer #8352

OjusWiZard commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 6, 2024 •

edited

Loading

yabirgb Aug 13, 2024

yabirgb Aug 13, 2024

yabirgb Aug 14, 2024

OjusWiZard Aug 14, 2024

yabirgb Aug 14, 2024

OjusWiZard Aug 14, 2024

yabirgb Aug 15, 2024

yabirgb Aug 16, 2024

		with database.user_write() as cursor:
		with database.conn.read_ctx() as cursor:

Refactor (Process): DB writer #8352

Are you sure you want to change the base?

Refactor (Process): DB writer #8352

Conversation

OjusWiZard commented Aug 2, 2024 • edited Loading

Checklist

codecov bot commented Aug 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OjusWiZard commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 6, 2024 •

edited

Loading