Testing zero-copy bugs fixes (not for merging) #1156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
133tosakarin
reviewed
Oct 1, 2024
| @@ -55,13 +59,56 @@ | |||
| */ | |||
| public class LeakDetector { | |||
| private static final Logger LOG = LoggerFactory.getLogger(LeakDetector.class); | |||
|
|
|||
| private static class LeakTrackerSet { | |||
| private final Set<LeakTracker> set = Collections.newSetFromMap(new ConcurrentHashMap<>()); | |||
There was a problem hiding this comment.
All methods are synchronized, do we still need to use ConcurrentHashMap?
There was a problem hiding this comment.
You are right! We don't need ConcurrentHashMap anymore.
133tosakarin
reviewed
Oct 7, 2024
| @@ -46,6 +47,8 @@ public class DataBlockingQueue<E> extends DataQueue<E> { | |||
| private final Condition notFull = lock.newCondition(); | |||
| private final Condition notEmpty = lock.newCondition(); | |||
|
|
|||
| private boolean closed = false; | |||
|
|
|||
There was a problem hiding this comment.
There is already a lock.
133tosakarin
approved these changes
Oct 7, 2024
| @@ -46,6 +47,8 @@ public class DataBlockingQueue<E> extends DataQueue<E> { | |||
| private final Condition notFull = lock.newCondition(); | |||
| private final Condition notEmpty = lock.newCondition(); | |||
|
|
|||
| private boolean closed = false; | |||
|
|
|||
|
Finally, it is able to pass all the tests (with a few retries). Note that there are probably some other zero copy bugs. Will fix them separately. |
|
This can pass all the tests (with a few retries). Since this change is quite big (56kB) and non-trivial, I will split this to a few JIRAs:
I will see if (2) and (3) needed to be further split. BTW, we should move |
szetszwo
changed the title
This was referenced Oct 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fix will be split into multiple JIRAs: RATIS-2164, RATIS-2151, RATIS-2173
The following are the bugs found so far:
LeakDetector: assertedallLeaksis non-empty but printed "allLeaks.size = 0"retain. Without callingretainat all, it is not a leak.SimpleTracingandAdvancedTracing: the methods should be synchronized.AdvancedTracingshould have a single track list instead ofretainsTracesandreleaseTraces.GrpcClientProtocolService.UnorderedRequestStreamObserver.processClientRequest(..)should use try-finally.GrpcLogAppender.appendLog(..)callsrelease()incorrectly for exception.LogAppenderDefault.sendAppendEntriesWithRetries(..)callsrelease()incorrectly for exception.LogSegmentcache can release an entry multiple times.LogSegment.loadCache(..)should callretain()for cache hit.SegmentedRaftLog.retainLog(..): between getting the entry and callingretain(), the entry can be released. The "fail to retain" exception, if there is any, can be ignored since It is the same as a cache miss. See RATIS-2159. TestRaftWithSimulatedRpc could "fail to retain". #1153SegmentedRaftLog.retainEntryWithData(..)should release for exception.SimpleStateMachine4Testingcan be released.LogSegment: New entries can be added after EntryCache is closed.MemoryRaftLoghas similar problems as inSegmentedRaftLog.SegmentedRaftLogWorkershould clean up unfinished tasks in the queue after stopped running.