Singleton global cluster cache limited by memory usage

[veloman-yunkan]

Fixes #947

This PR started as an effort to better test the enhancement of ConcurrentCache implemented in #956 and ended up superseding the latter after being broken up into smaller pieces (#974, #975, #982) that were merged separately whereupon it was easier to merge the remaining portion from this PR than rebasing/updating #956.

[mgautierfr]

(I only have a global look, not a thorough review of each commits)

Testing that something works correctly through logging seems a bit odd to me.
Log should be to get information about what happens, not to test that something happens in the correct order. A best we could call this tracing. It is a small semantic subtlety but I had to express it.

I am also uncomfortable with the orchestration. I was about to ask you how you handle the unpredictability of the output ordering caused by multithreading until I realize you orchestrate the thread based on the expected output. But doing so, you "linearize" the code workflow. The log system is locking threads to ensure that only one thread can log at the same time. So testing is changing the behavior of what is tested (mostly, locks in the concurrent cache become somehow useless) and we are not testing what will run in production.

[veloman-yunkan]

I am also uncomfortable with the orchestration. I was about to ask you how you handle the unpredictability of the output ordering caused by multithreading until I realize you orchestrate the thread based on the expected output. But doing so, you "linearize" the code workflow.

That is exactly the trick. Each instantiation of a mutlithreaded execution is equivalent to some linearization of it (assuming that there are no race conditions, i.e. access to shared resources is properly protected). Then I can define in the tests the linearizations that the code should handle.

The log system is locking threads to ensure that only one thread can log at the same time. So testing is changing the behavior of what is tested (mostly, locks in the concurrent cache become somehow useless) and we are not testing what will run in production.

Consider the situation that you currently have in ConcurrentCache - in a comment in ConcurrentCache::getOrPut() you talk in length about a rare situation that may happen during a tiny time window. My approach allows to simulate that scenario - we only need to put a log/trace statement in the said window and, based on our understanding of how that scenario should work, generate the log manually. The test's job will be to validate that the code indeed executes along that path.

More generally, tests for concurrent flows must be written so as to cover various linearizations. In a TDD approach, such tests can help producing the right code for concurrency management.

You are right that the approach lacks means for detecting absence of correct synchronization between threads. Thanks for pointing this out! I was mostly focused on testing the logic governing the execution under different scenarios, assuming that all measures against race conditions are in place. It should still allows to catch bugs leading to deadlocks or livelocks. I will think how that approach (possibly, after some enhancement) can be utilized to catch issues with potential race conditions.

[mgautierfr]

Consider the situation that you currently have in ConcurrentCache - in a comment in ConcurrentCache::getOrPut() you talk in length about a rare situation that may happen during a tiny time window. My approach allows to simulate that scenario - we only need to put a log/trace statement in the said window and, based on our understanding of how that scenario should work, generate the log manually. The test's job will be to validate that the code indeed executes along that path.

More generally, tests for concurrent flows must be written so as to cover various linearizations. In a TDD approach, such tests can help producing the right code for concurrency management.

Said like that I agree with you. But then I would prefer a proper name (as test_sync_barrier) instead of log. Especially as this will be noop at runtime and the purpose is to modify the behavior when we are testing.

[veloman-yunkan]

@mgautierfr I am done with the preliminary version of my contribution to your PR. I will have to clean it up significantly. But its second part (focusing on the testing of ConcurrentCache, starting from 6aa5aee) will be almost unaffected, so I invite you to look at it from there.

[veloman-yunkan]

Said like that I agree with you. But then I would prefer a proper name (as test_sync_barrier) instead of log. Especially as this will be noop at runtime and the purpose is to modify the behavior when we are testing.

Syncrhonization is only one of the uses of logging/tracing. Currently it is done with two logging statements (before and after the mutex locking operation) that must kept together in the target log of the simulated flow. A test_sync_barrier() macro may help to avoid the pitfall of trying to achieve that effect with a single simple (general purpose) logging statement. log_debug_raii_sync_statement() is what can be used for that purpose.

But logging also helps to visualize the operation of what is otherwise difficult to tell (for example, absence of double-work addressed by the last commit).

[kelson42]

@veloman-yunkan What is the status here? If I get it right you have extended/improve the PR of @mgautierfr (or code around it). I have nothing against it, but it is important we move forward and this PR seems stuck.

[veloman-yunkan]

@veloman-yunkan What is the status here? If I get it right you have extended/improve the PR of @mgautierfr (or code around it). I have nothing against it, but it is important we move forward and this PR seems stuck.

@kelson42 I am waiting for @mgautierfr's feedback.

[kelson42]

@mgautierfr Can you please come back here so we can move forward?

[mgautierfr]

I haven't made a full review commit by commit but I mostly agree with the proposition. The concurrent cache is better tested with this PR.

I reiterate that I am a bit unconfident with the log function/macro doing more than logging.
I'm afraid that in a near future, we start add log in other module or simply change the log in concurrent cache as we want to debug/test it manually. That's why I think a name as log_and_testing_sync would be a better name.

I am done with the preliminary version of my contribution to your PR. I will have to clean it up significantly.

Please clean it up so I can make a full review.

[kelson42]

Goal is to merge this PR and #956 so we can finally conclude on this memory mgmt effort.

[veloman-yunkan]

Rebased my branch including the changes from @mgautierfr's branch memory_cache on main, resolving conflicts with #958. Now going to clean-up the changes and split it into several PRs.

[veloman-yunkan]

Extracted the log_debug*() utilities into a separate (cleaned-up) PR #975. However the macros were not renamed since that would require more time and is contrary to @kelson42's objective of finishing this functionality ASAP.

[kelson42]

@mgautierfr @veloman-yunkan Once #975 merged, are we ready to merge this one?

• It seems not rebased on Log debug utils #975 feature branch
• maybe we have the time to rename the macros? Should be that lon.
• I woukd prefer to have a formal review

[veloman-yunkan]

@mgautierfr @veloman-yunkan Once #975 merged, are we ready to merge this one?

No, this PR has to be cleaned up and split.

• It seems not rebased on Log debug utils #975 feature branch

Yes, after you rebased the log_debug_utils branch after merging #976 this PR got behind. Now I rebased this PR again.

• maybe we have the time to rename the macros? Should be that lon.

I've thought about that too, but can't come up with good names.

• I woukd prefer to have a formal review

This PR should be reviewed after clean up/split.

[codecov]

Codecov Report

❌ Patch coverage is 73.17073% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.69%. Comparing base (30bac7d) to head (f4b6139).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
src/fileimpl.cpp	63.63%	1 Missing and 11 partials ⚠️
src/concurrent_cache.h	40.00%	0 Missing and 3 partials ⚠️
src/buffer_reader.cpp	0.00%	2 Missing ⚠️
src/compression.cpp	66.66%	1 Missing and 1 partial ⚠️
src/lrucache.h	77.77%	0 Missing and 2 partials ⚠️
src/cluster.cpp	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #960      +/-   ##
==========================================
+ Coverage   57.65%   57.69%   +0.04%     
==========================================
  Files         103      103              
  Lines        4926     4976      +50     
  Branches     2068     2083      +15     
==========================================
+ Hits         2840     2871      +31     
- Misses        706      713       +7     
- Partials     1380     1392      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[veloman-yunkan]

Work done in this PR (clean and properly tested enhancement of ConcurrentCache with support for user-defined item cost) has been extracted into #982 which should be merged before #956 or this very PR.

[kelson42]

@veloman-yunkan Not sure if #956 should be merged before this one or not, but I have rebased the branch on main HEAD and I guess this would be ready to merge?

[veloman-yunkan]

@veloman-yunkan Not sure if #956 should be merged before this one or not, but I have rebased the branch on main HEAD and I guess this would be ready to merge?

This PR (together with the already merged pieces extracted from it) overrides #956. It requires some clean-up and fixing of CI. Will be ready today.

[veloman-yunkan]

This PR is ready to be merged.