-
Notifications
You must be signed in to change notification settings - Fork 143
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: 17219: WARN EXCEPTION "virtual-map: cache-cleaner StandardFuture: Future has already been cancelled" #17475
base: main
Are you sure you want to change the base?
fix: 17219: WARN EXCEPTION "virtual-map: cache-cleaner StandardFuture: Future has already been cancelled" #17475
Conversation
…: Future has already been cancelled" Signed-off-by: Artem Ananev <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17475 +/- ##
============================================
- Coverage 69.01% 68.96% -0.05%
- Complexity 22724 22761 +37
============================================
Files 2615 2619 +4
Lines 98026 98269 +243
Branches 10162 10185 +23
============================================
+ Hits 67649 67774 +125
- Misses 26559 26663 +104
- Partials 3818 3832 +14
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
.../swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/cache/ConcurrentArray.java
Outdated
Show resolved
Hide resolved
@@ -1434,11 +1434,11 @@ private Mutation<K, VirtualLeafRecord<K, V>> mutate( | |||
* @param <V> | |||
* The value type referenced by the mutation list | |||
*/ | |||
private static <K, V> void purge( | |||
private static <K, V> StandardFuture<Void> purge( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You change the return value type but never use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a leftover from my first approach to this issue. I was going to use this StandardFuture and call getAndRethrow()
on it in release()
, but gave up with this idea. array.parallelTraverse()
returns a StandardFuture anyway
dirtyLeafPaths = null; | ||
dirtyHashes = null; | ||
}); | ||
purge(dirtyLeaves, keyToDirtyLeafIndex, virtualMapConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is performance impact of calling purge()
synchronously negligible? What is the benefit of the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing purge()
does is it calls array.parallelTraverse(), which runs everything in the node cache cleaning pool. There is no need to run purge()
itself in the same pool, it just complicates the code.
…in ConcurrentArray.parallelTraverse() Signed-off-by: Artem Ananev <[email protected]>
Fixes: #17219
Signed-off-by: Artem Ananev [email protected]