Skip to content

Tests: No Cache Testing in ledger#5058

Merged
jannotti merged 34 commits into
algorand:masterfrom
ahangsu:no-cache-testing
Feb 17, 2023
Merged

Tests: No Cache Testing in ledger#5058
jannotti merged 34 commits into
algorand:masterfrom
ahangsu:no-cache-testing

Conversation

@ahangsu
Copy link
Copy Markdown
Contributor

@ahangsu ahangsu commented Jan 25, 2023

Summary

Had a discussion with Pavel on disabling ledger LRU cache for testing purpose, we settled down on following design/implementation:

  • Add a boolean field DisableLedgerLRUCache in Local (config/localTemplate.go)
  • On accountUpdates/onlineAccounts initialize, maintain disableCache = cfg.DisableLedgerLRUCache.
  • On accountUpdates/onlineAccounts initializeFromDisk, if !initializeFromDisk, init lrukv/lruaccts/lruresources/lruOnlineAccounts with pendingWrite = 0.
  • Internal each LRU cache implementation, if pendingWrite == 0, set disableWrite* to be True.
  • Internal each LRU cache implementation, if disableWrite* is True, then bypasses the write operation, such that it never writes into kvs (or map structure alike).

Test Plan

Need to design on testcase to confirm that it works...

  • Unit test on 4 LRU implementation to make sure that disabling them will let no write into the cache.
  • One manual negative test against accountUpdate in commit: f5f2af3, which confirmed that disabling LRU cache will lead to reading against DB, thus the test is indeed testing something.

@ahangsu ahangsu added the test Improves testing of existing code label Jan 25, 2023
@ahangsu ahangsu changed the title No Cache Testing in ledger Tests: No Cache Testing in ledger Jan 25, 2023
@ahangsu ahangsu force-pushed the no-cache-testing branch 2 times, most recently from 7f1b4a8 to 074cac2 Compare January 25, 2023 21:15
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 25, 2023

Codecov Report

Merging #5058 (5954351) into master (6487b37) will increase coverage by 0.00%.
The diff coverage is 74.66%.

@@           Coverage Diff           @@
##           master    #5058   +/-   ##
=======================================
  Coverage   53.42%   53.42%           
=======================================
  Files         431      432    +1     
  Lines       54369    54412   +43     
=======================================
+ Hits        29046    29071   +25     
- Misses      23061    23079   +18     
  Partials     2262     2262           
Impacted Files Coverage Δ
config/localTemplate.go 63.26% <ø> (ø)
ledger/ledger.go 70.09% <ø> (ø)
ledger/testing/withAndWithoutCache.go 0.00% <0.00%> (ø)
ledger/testing/consensusRange.go 23.07% <35.29%> (+23.07%) ⬆️
ledger/acctonline.go 78.86% <100.00%> (+0.22%) ⬆️
ledger/acctupdates.go 69.46% <100.00%> (+0.46%) ⬆️
ledger/lruaccts.go 92.45% <100.00%> (+2.86%) ⬆️
ledger/lrukv.go 94.59% <100.00%> (+0.84%) ⬆️
ledger/lruonlineaccts.go 94.59% <100.00%> (+0.84%) ⬆️
ledger/lruresources.go 92.85% <100.00%> (+2.66%) ⬆️
... and 12 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment thread ledger/acctupdates_test.go Outdated
require.Equal(t, testcases[0].boxValue, val)
addKV2AU(testcases[4].appID, testcases[4].boxKey, testcases[4].boxValue)
_, err = readKey(testcases[0].appID, testcases[0].boxKey)
require.NoError(t, err)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is the place that will hit error, for first box (testcase[0]) goes into db after kvdelta:

  • with LRU cache on, account-update reads from cache and still can get result, so it will not hit db read.
  • otherwise, with nothing read from LRU cache and kvdelta, goes into DB and trigger panic.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great. thanks for this sanity check.

Comment thread ledger/testing/consensusRange.go Outdated
algorandskiy
algorandskiy previously approved these changes Feb 10, 2023
@ahangsu ahangsu requested a review from cce February 10, 2023 16:30
Comment thread ledger/acctupdates.go
Comment thread ledger/lruaccts.go Outdated
Comment thread ledger/acctupdates_test.go
Comment thread ledger/lruaccts.go Outdated
Comment thread ledger/lruaccts.go
Comment thread ledger/testing/consensusRange.go Outdated
Comment thread ledger/testing/consensusRange.go Outdated
jannotti
jannotti previously approved these changes Feb 15, 2023
Copy link
Copy Markdown
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

Had some minor comments, but I approve. Let me know when you want it merged.

@ahangsu
Copy link
Copy Markdown
Contributor Author

ahangsu commented Feb 15, 2023

Thanks for the previous approval @jannotti !

Let's see if I can get Chris approving it tomorrow, then we merge with 2 approvals.

Comment thread ledger/testing/consensusRange.go
@jannotti jannotti merged commit e140542 into algorand:master Feb 17, 2023
@ahangsu ahangsu deleted the no-cache-testing branch February 17, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement test Improves testing of existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants