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.

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

Fix: Disable LRU flushPendingWrite warning if disabled #5184

Merged
merged 3 commits into from
Mar 9, 2023

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Mar 8, 2023

Summary

Per Pavel's request, disable flushPendingWrite warning if LRU cache is disabled.

ledger/lruaccts.go Outdated Show resolved Hide resolved
@ahangsu ahangsu requested review from algorandskiy and cce March 8, 2023 20:40
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #5184 (d863626) into master (4bbedc0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5184      +/-   ##
==========================================
- Coverage   53.51%   53.50%   -0.01%     
==========================================
  Files         439      439              
  Lines       54985    54985              
==========================================
- Hits        29423    29422       -1     
- Misses      23271    23274       +3     
+ Partials     2291     2289       -2     
Impacted Files Coverage Δ
ledger/acctonline.go 77.66% <100.00%> (ø)
ledger/acctupdates.go 69.12% <100.00%> (+0.23%) ⬆️
cmd/tealdbg/debugger.go 71.65% <0.00%> (-0.79%) ⬇️
ledger/catchpointtracker.go 55.89% <0.00%> (-0.77%) ⬇️
network/wsNetwork.go 68.90% <0.00%> (-0.18%) ⬇️
catchup/service.go 70.82% <0.00%> (ø)
ledger/testing/randomAccounts.go 56.26% <0.00%> (ø)
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️

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

algorandskiy
algorandskiy previously approved these changes Mar 8, 2023
@@ -68,9 +68,9 @@ const (
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should not be changed in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, reverting this and separate out for another PR then

@ahangsu ahangsu changed the title Enhancement: Disable LRU flushPendingWrite warning if disabled, update some comments in agreement Enhancement: Disable LRU flushPendingWrite warning if disabled Mar 8, 2023
Copy link
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.

I would rather see the calls that initiialise the caches in disabled mode, which currently look like this:

		au.baseAccounts.init(au.log, 0, 0)
		au.baseResources.init(au.log, 0, 0)
		au.baseKVs.init(au.log, 0, 0)

changed to

		au.baseAccounts.init(au.log, 0, 1)
		au.baseResources.init(au.log, 0, 1)
		au.baseKVs.init(au.log, 0, 1)

which implicitly turns off the warning, since the threshold is higher than the size.
That avoids the extra test all the time.

@ahangsu
Copy link
Contributor Author

ahangsu commented Mar 9, 2023

yea, that's fair, I will flip the change asap.

@ahangsu ahangsu requested a review from jannotti March 9, 2023 16:04
@jannotti jannotti merged commit ce375b1 into algorand:master Mar 9, 2023
@ahangsu ahangsu deleted the lru-skip-warning-if-disabled branch March 9, 2023 16:32
@algorandskiy algorandskiy changed the title Enhancement: Disable LRU flushPendingWrite warning if disabled Fix: Disable LRU flushPendingWrite warning if disabled Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants