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

Aggregate Universe Stats: always store result of querySyncStats #1302

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

GeorgeTsagk
Copy link
Member

@GeorgeTsagk GeorgeTsagk commented Jan 16, 2025

Our universe stats could run into a case where if in an un-cached state (like on system start-up) the db query takes longer than the default client timeout (usually 30s) then we'd fail the query and return early. This would cause the next client to also receive an error instead of the cached result, making this codepath stuck in an endless failing query. With this change we parallelize the db query and we always wait for the result in order to store it in cache. We may fail the function earlier, but the result will always be retrieved & persisted. This way, even if the 1st client retrieves an error, the next one will receive the cached (late) result of the previous run.

Closes #1304

@GeorgeTsagk GeorgeTsagk self-assigned this Jan 16, 2025
@coveralls
Copy link

coveralls commented Jan 16, 2025

Pull Request Test Coverage Report for Build 12904863504

Details

  • 34 of 39 (87.18%) changed or added relevant lines in 1 file are covered.
  • 24 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.04%) to 40.807%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapdb/universe_stats.go 34 39 87.18%
Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
commitment/tap.go 2 84.32%
asset/asset.go 2 77.13%
tapgarden/caretaker.go 4 68.5%
asset/mock.go 4 92.13%
universe/interface.go 10 52.81%
Totals Coverage Status
Change from base Build 12893024407: 0.04%
Covered Lines: 26697
Relevant Lines: 65423

💛 - Coveralls

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Concept ACK. Some kind of test case (unit or integration) would be nice.

tapdb/universe_stats.go Outdated Show resolved Hide resolved
tapdb/universe_stats.go Outdated Show resolved Hide resolved
Our universe stats could run into a case where if in an un-cached state
the db query takes longer than the default client timeout (usually 30s)
then we'd fail the query and return early. This would cause the next
client to also receive an error instead of the cached result, making
this codepath stuck in an endless failing query. With this change we
parallelize the db query and we always wait for the result in order to
store it in cache. We may fail the function earlier, but the result will
always be retrieved & persisted.
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice!

tapdb/universe_stats_test.go Show resolved Hide resolved
With this commit we add a simple test for the asynchronous cache
population that was introduced previously. We make a single RPC call to
the aggregate stats endpoint with a client timeout that is too small.
This leads to an RPC failure, so we check whether the cache was updated
in the background.
@GeorgeTsagk GeorgeTsagk requested review from ffranr and removed request for Roasbeef January 22, 2025 09:13
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Nice!

@ffranr ffranr added this pull request to the merge queue Jan 22, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jan 22, 2025
Aggregate Universe Stats: always store result of querySyncStats
Merged via the queue into lightninglabs:main with commit 563efdc Jan 22, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[bug]: Universe aggregate stats cache is stuck on 0.00% hit ratio
4 participants