-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Flaky-test: MessageDispatchThrottlingTest.testBacklogConsumerCacheReads #16979
Comments
sometimes I can reproduce this in local machine It should be 200*10 I print some log in messagelistener. It seems there has a bug in recent change
|
@leizhiyuan Do you want to create a PR to fix the issue? |
ok |
* feat:return true if entry exists in cache * feat:return true if entry exists in cache * chore: add comment Co-authored-by: zhiyuanlei <zhiyuanlei@tencent.com> Master Issue: #16979 ### Motivation PR #12258 made changes to OpAddEntry which causes a memory leak when the entry is already in the cache. ### Modifications Modifications: Revert PR #12258 changes to OpAddEntry ### Documentation - [x] `doc-not-needed`
* feat:return true if entry exists in cache * feat:return true if entry exists in cache * chore: add comment Co-authored-by: zhiyuanlei <zhiyuanlei@tencent.com> Master Issue: #16979 ### Motivation PR #12258 made changes to OpAddEntry which causes a memory leak when the entry is already in the cache. ### Modifications Modifications: Revert PR #12258 changes to OpAddEntry ### Documentation - [x] `doc-not-needed` (cherry picked from commit 374b3a1)
* feat:return true if entry exists in cache * feat:return true if entry exists in cache * chore: add comment Co-authored-by: zhiyuanlei <zhiyuanlei@tencent.com> Master Issue: apache#16979 ### Motivation PR apache#12258 made changes to OpAddEntry which causes a memory leak when the entry is already in the cache. ### Modifications Modifications: Revert PR apache#12258 changes to OpAddEntry ### Documentation - [x] `doc-not-needed` (cherry picked from commit 374b3a1)
Looks like this problem was fixed by PR #16996 |
I just ran into the following error while running
|
Reopening this ticket because I am consistently getting this test to fail on my local machine. I also just go this variant:
Seems like we have a data race. Still digging. |
Seems like this might be related to #10924. |
#10924 is not related. Just before calling the pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java Lines 436 to 446 in 926834e
|
It's clear to me now that the issue is a data race with the Lines 554 to 555 in abff91f
The fundamental race comes from the caching. When an entry is expired from the cache, we call Here is another stack trace I found from running the test. It looks like we already found similar stack traces here #14436. My analysis aligns with @lhotari's. I'll have a PR up soon.
|
I wrote michaeljmarshall@8f48cf2 thinking that it would solve the issue. However, I am still getting the same exceptions, so there must be something I am missing. I'll continue on this tomorrow or Monday. |
…17105) Fixes #16979 ### Motivation #12258 introduced caching for backlogged consumers. When caching the entry, it is important to duplicate the `ByteBuffer` so that the reader index is not shared. The current code has a race condition where the `ByteBuffer` reference in the cache is shared with the dispatcher. When another consumer reads from the cache, the cache calls `duplicate()` on the shared `ByteBuffer`, which copies the current reader index, which might not be 0 if the original dispatcher read data from the `ByteBuffer`. Note: it seems like the caching `insert` method creates (or recycles) more `EntryImpl` instances than is really necessary. Changing that is outside this PR's scope, so I am going to leave it as is. ### Modifications * Create a new `Entry` before inserting it into the cache. * Add a new test to the `EntryCacheTest`. The test fails before this change and passes after it. * Update the `EntryCacheTest` mocking so that it returns unique entries when mocking reads from the bookkeeper. Before, all returned `LedgerEntry` objects had ledgerId 0 and entryId 0, which messed with the caching for the new test. ### Verifying this change This change includes a test that failed before the PR and passes after it. ### Documentation - [x] `doc-not-needed`
…17105) Fixes #16979 ### Motivation #12258 introduced caching for backlogged consumers. When caching the entry, it is important to duplicate the `ByteBuffer` so that the reader index is not shared. The current code has a race condition where the `ByteBuffer` reference in the cache is shared with the dispatcher. When another consumer reads from the cache, the cache calls `duplicate()` on the shared `ByteBuffer`, which copies the current reader index, which might not be 0 if the original dispatcher read data from the `ByteBuffer`. Note: it seems like the caching `insert` method creates (or recycles) more `EntryImpl` instances than is really necessary. Changing that is outside this PR's scope, so I am going to leave it as is. ### Modifications * Create a new `Entry` before inserting it into the cache. * Add a new test to the `EntryCacheTest`. The test fails before this change and passes after it. * Update the `EntryCacheTest` mocking so that it returns unique entries when mocking reads from the bookkeeper. Before, all returned `LedgerEntry` objects had ledgerId 0 and entryId 0, which messed with the caching for the new test. ### Verifying this change This change includes a test that failed before the PR and passes after it. ### Documentation - [x] `doc-not-needed` (cherry picked from commit 76f4195)
…pache#17105) Fixes apache#16979 ### Motivation apache#12258 introduced caching for backlogged consumers. When caching the entry, it is important to duplicate the `ByteBuffer` so that the reader index is not shared. The current code has a race condition where the `ByteBuffer` reference in the cache is shared with the dispatcher. When another consumer reads from the cache, the cache calls `duplicate()` on the shared `ByteBuffer`, which copies the current reader index, which might not be 0 if the original dispatcher read data from the `ByteBuffer`. Note: it seems like the caching `insert` method creates (or recycles) more `EntryImpl` instances than is really necessary. Changing that is outside this PR's scope, so I am going to leave it as is. ### Modifications * Create a new `Entry` before inserting it into the cache. * Add a new test to the `EntryCacheTest`. The test fails before this change and passes after it. * Update the `EntryCacheTest` mocking so that it returns unique entries when mocking reads from the bookkeeper. Before, all returned `LedgerEntry` objects had ledgerId 0 and entryId 0, which messed with the caching for the new test. ### Verifying this change This change includes a test that failed before the PR and passes after it. ### Documentation - [x] `doc-not-needed` (cherry picked from commit 76f4195)
* feat:return true if entry exists in cache * feat:return true if entry exists in cache * chore: add comment Co-authored-by: zhiyuanlei <zhiyuanlei@tencent.com> Master Issue: apache#16979 ### Motivation PR apache#12258 made changes to OpAddEntry which causes a memory leak when the entry is already in the cache. ### Modifications Modifications: Revert PR apache#12258 changes to OpAddEntry ### Documentation - [x] `doc-not-needed`
…pache#17105) Fixes apache#16979 ### Motivation apache#12258 introduced caching for backlogged consumers. When caching the entry, it is important to duplicate the `ByteBuffer` so that the reader index is not shared. The current code has a race condition where the `ByteBuffer` reference in the cache is shared with the dispatcher. When another consumer reads from the cache, the cache calls `duplicate()` on the shared `ByteBuffer`, which copies the current reader index, which might not be 0 if the original dispatcher read data from the `ByteBuffer`. Note: it seems like the caching `insert` method creates (or recycles) more `EntryImpl` instances than is really necessary. Changing that is outside this PR's scope, so I am going to leave it as is. ### Modifications * Create a new `Entry` before inserting it into the cache. * Add a new test to the `EntryCacheTest`. The test fails before this change and passes after it. * Update the `EntryCacheTest` mocking so that it returns unique entries when mocking reads from the bookkeeper. Before, all returned `LedgerEntry` objects had ledgerId 0 and entryId 0, which messed with the caching for the new test. ### Verifying this change This change includes a test that failed before the PR and passes after it. ### Documentation - [x] `doc-not-needed`
example failure
The text was updated successfully, but these errors were encountered: