XRPLF / clio

An XRP Ledger API Server
https://xrpl.org
ISC License
56 stars 48 forks source link

Disable cache on missing data #1368

Closed godexsoft closed 2 months ago

godexsoft commented 2 months ago

For #1354

This is the first part of the issue. The second part will have to wait and be implemented once we have SHAMap available as a library in Clio.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 53.84615% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 64.45%. Comparing base (82b8316) to head (a5769d9).

Files Patch % Lines
src/etl/CorruptionDetector.hpp 50.00% 0 Missing and 3 partials :warning:
src/rpc/handlers/ServerInfo.hpp 40.00% 0 Missing and 3 partials :warning:
src/data/LedgerCache.cpp 66.66% 1 Missing and 1 partial :warning:
src/etl/ETLService.hpp 0.00% 2 Missing :warning:
src/etl/ETLService.cpp 0.00% 1 Missing :warning:
src/etl/impl/CacheLoader.hpp 50.00% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1368 +/- ## =========================================== + Coverage 64.42% 64.45% +0.02% =========================================== Files 250 251 +1 Lines 10576 10598 +22 Branches 5855 5866 +11 =========================================== + Hits 6814 6831 +17 + Misses 1921 1916 -5 - Partials 1841 1851 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cindyyan317 commented 2 months ago

Nice wrote ! Found an edge case, please recheck if it is valid. I am still thinking we should disable all cache usage or just for ETL? 🤔
If we disable all cache usage, it makes the problem visible to users. Maybe it is better choice. But it indeed affects the performance.

godexsoft commented 2 months ago

Nice wrote ! Found an edge case, please recheck if it is valid. I am still thinking we should disable all cache usage or just for ETL? 🤔 If we disable all cache usage, it makes the problem visible to users. Maybe it is better choice. But it indeed affects the performance.

I'd think let's disable it entirely for now. If we find it become an issue we can always adjust it later.