Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: implement dht pooled db connection #3596

Merged
merged 8 commits into from
Nov 24, 2021
Merged

feat: implement dht pooled db connection #3596

merged 8 commits into from
Nov 24, 2021

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Nov 19, 2021 •
edited
Loading

Description

Implemented the pooled SQLite db connection for the DHT storage in favour of the Rust based async read-write lock, which makes use of the much faster SQLite3 Write-ahead Log (WAL) mode.

Motivation and Context

Recent changes to the common implementation for SQLite pooled connections managed in the background via Diesel and SQLite3 enabled this improvement.

How Has This Been Tested?

  • Unit tests
  • Cucumber tests
  • System-level tests [Edit: Running a twice more intensive stress test than before (2x sender wallets with 3x 1000 transactions on one computer to 3x receiver wallets on another computer vs. 1x sender wallet with 3x 1000 transactions to 3x receiver wallets) the disk resource utilization was 90+% down from previous levels - ~100% disk utilization on both systems during all phases of the transactions life cycle down to <~5% with occasional spikes up to ~80%. All transactions reached mined confirmed in all 5 participating wallets.)

sdbondi
sdbondi reviewed Nov 19, 2021
View reviewed changes
Copy link
Member

@sdbondi sdbondi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, A little concerned about blocking tokio task scheduling with synchronous calls, but understand that may be tricky with pooled connections. Let's see if we end up with any perf issues - it's in the wallet, so will probably be fine.

comms/dht/src/dedup/dedup_cache.rs Outdated Show resolved Hide resolved
comms/dht/src/storage/error.rs Outdated Show resolved Hide resolved
comms/dht/src/store_forward/database/mod.rs Outdated Show resolved Hide resolved
@hansieodendaal hansieodendaal force-pushed the ho_dht_pooled_db_connection branch from 274af28 to 0163a2d Compare November 22, 2021 08:23
@hansieodendaal
Copy link
Contributor Author

Looks good, A little concerned about blocking tokio task scheduling with synchronous calls, but understand that may be tricky with pooled connections. Let's see if we end up with any perf issues - it's in the wallet, so will probably be fine.

@sdbondi see edit about stress test results monitoring disk utilization. The change actually affects both the base node's and the wallet's DHT dedup databases.

@sdbondi
Copy link
Member

sdbondi commented Nov 23, 2021 •
edited
Loading

@hansieodendaal I'm happy about using a WAL - was talking specifically about tokio scheduler blocking, but if the calls are quick enough it shouldn't have a negative impact. I expect it will be fine.

EDIT: oh just saw the updated PR description - LGTM

@sdbondi
Copy link
Member

sdbondi commented Nov 23, 2021

@hansieodendaal Just need to address the infinite recursion in PartialEq

@hansieodendaal hansieodendaal force-pushed the ho_dht_pooled_db_connection branch from 0163a2d to 1b41cf2 Compare November 23, 2021 10:35
@hansieodendaal hansieodendaal force-pushed the ho_dht_pooled_db_connection branch from 1b41cf2 to c38fff2 Compare November 23, 2021 14:37
sdbondi
sdbondi previously approved these changes Nov 23, 2021
View reviewed changes
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice looks good

philipr-za
philipr-za previously approved these changes Nov 23, 2021
View reviewed changes
StriderDM and others added 5 commits November 24, 2021 08:48
@StriderDM @hansieodendaal
feat: display network for console wallet
c92540e
@hansieodendaal
Implement dht pooled db connection
577b087 
Implemented the pooled sqlite db connection for the dht storage in
favour of the Rust based async read-write lock.
@hansieodendaal
Review comments
713a92e
@hansieodendaal
review comments
d6b2cab
@hansieodendaal
review comments
a817669
@hansieodendaal hansieodendaal dismissed stale reviews from philipr-za and sdbondi via a817669 November 24, 2021 07:10
@hansieodendaal hansieodendaal force-pushed the ho_dht_pooled_db_connection branch from c38fff2 to a817669 Compare November 24, 2021 07:10
@aviator-app aviator-app bot merged commit 2ac0757 into tari-project:development Nov 24, 2021
@hansieodendaal hansieodendaal deleted the ho_dht_pooled_db_connection branch November 24, 2021 12:01
sdbondi added a commit to sdbondi/tari that referenced this pull request Nov 25, 2021
@sdbondi
Merge branch 'development' into core-prune-inputs
88eeed0 
* development:
  feat: implement dht pooled db connection (tari-project#3596)
  feat: add page for detailed mempool in explorer (tari-project#3613)
  chore: add pub key in the dailes notify (tari-project#3612)
  feat: display network for console wallet (tari-project#3611)
  docs: update covenants links (tari-project#3614)
sdbondi added a commit to sdbondi/tari that referenced this pull request Dec 6, 2021
@sdbondi
Merge branch 'development' into core-remove-double-db-lock
b5d5a5b 
* development: (29 commits)
  fix(pruned mode)!: prune inputs, allow horizon sync resume and other fixes (tari-project#3521)
  feat!: sending one-sided transactions in wallet_ffi (tari-project#3634)
  fix: use json 5 for tor identity (regression) (tari-project#3624)
  test: add operation_id to log messages (tari-project#3633)
  fix!: multiple monerod addresses in tari merge mining proxy (tari-project#3628)
  fix: get-peer command works with public key again (tari-project#3636)
  fix!: separate peer seeds to common.network (tari-project#3635)
  test: removed stress test log target (tari-project#3631)
  feat: removed transaction validation redundant events (tari-project#3630)
  feat: improve wallet responsiveness (tari-project#3625)
  feat: add bulletproof rewind profiling (tari-project#3618)
  fix!: console wallet grpc_console_wallet_addresss config (tari-project#3619)
  test: increase timeout in cucumber (tari-project#3621)
  chore: change status line (tari-project#3610)
  feat!: add tcp bypass settings for tor in wallet_ffi (tari-project#3615)
  feat: only trigger UTXO scanning when a new block event is received (tari-project#3620)
  feat: implement dht pooled db connection (tari-project#3596)
  feat: add page for detailed mempool in explorer (tari-project#3613)
  chore: add pub key in the dailes notify (tari-project#3612)
  feat: display network for console wallet (tari-project#3611)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipr-za philipr-za philipr-za approved these changes

@sdbondi sdbondi sdbondi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hansieodendaal @sdbondi @philipr-za @StriderDM