[refactor][broker] Remove EntryWrapper usages and use MessageMetadata instead #15967
Conversation
This file contains hidden or 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
… instead ### Motivation apache#7266 introduced the `EntryWrapper` to store the `Entry` object and the associated `MessageMetadata` if it exists. However, the `getEntry` field of `EntryWrapper` is never used. There is no need to allocate memory for `EntryWrapper`, even if it's allocated from the recycler pool. ### Modifications - Calculate the remaining messages without creating `EntryWrapper` object, just iterate over the parsed message metadata list. - Pass an optional `MessageMetadata` array to `filterEntriesForConsumer` and add the JavaDocs for these two parameters. After that, Remove unused `EntryWrapper` and `updateEntryWrapperWithMetadata`. This PR uses functional programming style to make code more simple and clear. ### Verifying this change - [ ] Make sure that the change passes the CI checks. This change is already covered by existing tests, such as `testBatchMessageDispatchingAccordingToPermits`. ### Documentation Check the box below or label this PR directly. Need to update docs? - [ ] `doc-required` (Your PR needs to update docs and you will update later) - [x] `doc-not-needed` (Please explain why) - [ ] `doc` (Your PR contains doc changes) - [ ] `doc-complete` (Docs have been already added)
BewareMyPower
requested review from
Jason918,
codelipenghui,
eolivelli,
hangc0276,
merlimat and
rdhabalia
BewareMyPower
added
area/broker
type/refactor
codelipenghui
approved these changes
Jun 8, 2022
Contributor
|
@BewareMyPower @codelipenghui please give time for other reviewers to see such important patches. I believe that for non trivial patches we must at least give 1 day for people to see what's going on. |
Contributor
|
@eolivelli Yes, agree. |
Contributor
Author
|
Sorry for the quick merge since this is a part of my development that I want to split it into multiple PRs. @eolivelli And unfortunately, just now I found this PR introduced a bug that I will fix it soon. |
Contributor
Author
|
After digging for a while, this PR didn't introduce a bug but it changed the original behavior a little and could bring unexpected behavior. I'll open a new PR and explain it in detail. |
BewareMyPower
added a commit
to BewareMyPower/pulsar
that referenced
this pull request
Jun 8, 2022
…peekMessageMetadata` ### Motivation apache#15967 removed the `EntryWrapper`, which holds an `Entry` instance that is never used. Instead, after the refactoring, the `MessageMetadata` array is useless, see https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L517-L519 Each `MessageMetadata` instance in the array is returned by `peekMessageMetadata`, whose returned value references a thread local object `Commands#LOCAL_MESSAGE_METADATA`. It brings a problem that if multiple entries were read, all `MessageMetadata` elements in the array reference the same object. However, accidentally, the wrong invocation of `Optional#orElse` saves it. See https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L133-L134 Each time `peekMessageMetadata` is called, the thread local message metadata will be updated. Unlike `orElseGet`, the expression in `orElse` is always called no matter if the optional is empty. This behavior change increases the invocations count of `peekMessageMetadata` and the `metadataArray` cache became redundant. ### Modifications - Use `orElseGet` instead of `orElse` in `AbstractBaseDispatcher`. - Add a new static method `Commands#peekAndCopyMessageMetadata` that returns a `MessageMetadata` instance allocated from heap memory. - Call `peekAndCopyMessageMetadata` to cache all message metadata instances in `PersistentDispatcherMultipleConsumers`. ### Verifying this change - [ ] Make sure that the change passes the CI checks. It's hard to add tests. As I've explained before, apache#15967 only degrades the performance and doesn't affect the correctness. ### Documentation Check the box below or label this PR directly. Need to update docs? - [ ] `doc-required` (Your PR needs to update docs and you will update later) - [x] `doc-not-needed` (Please explain why) - [ ] `doc` (Your PR contains doc changes) - [ ] `doc-complete`
5 tasks
BewareMyPower
added a commit
that referenced
this pull request
Jun 10, 2022
…peekMessageMetadata` (#15983) ### Motivation #15967 removed the `EntryWrapper`, which holds an `Entry` instance that is never used. Instead, after the refactoring, the `MessageMetadata` array is useless, see https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L517-L519 Each `MessageMetadata` instance in the array is returned by `peekMessageMetadata`, whose returned value references a thread local object `Commands#LOCAL_MESSAGE_METADATA`. It brings a problem that if multiple entries were read, all `MessageMetadata` elements in the array reference the same object. However, accidentally, the wrong invocation of `Optional#orElse` saves it. See https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L133-L134 Each time `peekMessageMetadata` is called, the thread local message metadata will be updated. Unlike `orElseGet`, the expression in `orElse` is always called no matter if the optional is empty. This behavior change increases the invocations count of `peekMessageMetadata` and the `metadataArray` cache became redundant. ### Modifications - Use `orElseGet` instead of `orElse` in `AbstractBaseDispatcher`. - Add a new static method `Commands#peekAndCopyMessageMetadata` that returns a `MessageMetadata` instance allocated from heap memory. - Call `peekAndCopyMessageMetadata` to cache all message metadata instances in `PersistentDispatcherMultipleConsumers`. ### Verifying this change It's hard to add tests. As I've explained before, #15967 only degrades the performance and doesn't affect the correctness.
eolivelli
pushed a commit
to datastax/pulsar
that referenced
this pull request
Sep 16, 2022
… instead (apache#15967) apache#7266 introduced the `EntryWrapper` to store the `Entry` object and the associated `MessageMetadata` if it exists. However, the `getEntry` field of `EntryWrapper` is never used. There is no need to allocate memory for `EntryWrapper`, even if it's allocated from the recycler pool. - Calculate the remaining messages without creating `EntryWrapper` object, just iterate over the parsed message metadata list. - Pass an optional `MessageMetadata` array to `filterEntriesForConsumer` and add the JavaDocs for these two parameters. After that, Remove unused `EntryWrapper` and `updateEntryWrapperWithMetadata`. This PR uses functional programming style to make code more simple and clear. (cherry picked from commit c33b12d)
eolivelli
pushed a commit
to datastax/pulsar
that referenced
this pull request
Sep 16, 2022
…peekMessageMetadata` (apache#15983) apache#15967 removed the `EntryWrapper`, which holds an `Entry` instance that is never used. Instead, after the refactoring, the `MessageMetadata` array is useless, see https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L517-L519 Each `MessageMetadata` instance in the array is returned by `peekMessageMetadata`, whose returned value references a thread local object `Commands#LOCAL_MESSAGE_METADATA`. It brings a problem that if multiple entries were read, all `MessageMetadata` elements in the array reference the same object. However, accidentally, the wrong invocation of `Optional#orElse` saves it. See https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L133-L134 Each time `peekMessageMetadata` is called, the thread local message metadata will be updated. Unlike `orElseGet`, the expression in `orElse` is always called no matter if the optional is empty. This behavior change increases the invocations count of `peekMessageMetadata` and the `metadataArray` cache became redundant. - Use `orElseGet` instead of `orElse` in `AbstractBaseDispatcher`. - Add a new static method `Commands#peekAndCopyMessageMetadata` that returns a `MessageMetadata` instance allocated from heap memory. - Call `peekAndCopyMessageMetadata` to cache all message metadata instances in `PersistentDispatcherMultipleConsumers`. It's hard to add tests. As I've explained before, apache#15967 only degrades the performance and doesn't affect the correctness. (cherry picked from commit 36690f5)
This was referenced Sep 16, 2022
eolivelli
pushed a commit
to datastax/pulsar
that referenced
this pull request
Sep 16, 2022
… instead (apache#15967) apache#7266 introduced the `EntryWrapper` to store the `Entry` object and the associated `MessageMetadata` if it exists. However, the `getEntry` field of `EntryWrapper` is never used. There is no need to allocate memory for `EntryWrapper`, even if it's allocated from the recycler pool. - Calculate the remaining messages without creating `EntryWrapper` object, just iterate over the parsed message metadata list. - Pass an optional `MessageMetadata` array to `filterEntriesForConsumer` and add the JavaDocs for these two parameters. After that, Remove unused `EntryWrapper` and `updateEntryWrapperWithMetadata`. This PR uses functional programming style to make code more simple and clear. (cherry picked from commit c33b12d)
eolivelli
pushed a commit
to datastax/pulsar
that referenced
this pull request
Sep 16, 2022
…peekMessageMetadata` (apache#15983) apache#15967 removed the `EntryWrapper`, which holds an `Entry` instance that is never used. Instead, after the refactoring, the `MessageMetadata` array is useless, see https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L517-L519 Each `MessageMetadata` instance in the array is returned by `peekMessageMetadata`, whose returned value references a thread local object `Commands#LOCAL_MESSAGE_METADATA`. It brings a problem that if multiple entries were read, all `MessageMetadata` elements in the array reference the same object. However, accidentally, the wrong invocation of `Optional#orElse` saves it. See https://github.com/apache/pulsar/blob/298a573295f845e46f8a55cee366b6db63e997c2/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java#L133-L134 Each time `peekMessageMetadata` is called, the thread local message metadata will be updated. Unlike `orElseGet`, the expression in `orElse` is always called no matter if the optional is empty. This behavior change increases the invocations count of `peekMessageMetadata` and the `metadataArray` cache became redundant. - Use `orElseGet` instead of `orElse` in `AbstractBaseDispatcher`. - Add a new static method `Commands#peekAndCopyMessageMetadata` that returns a `MessageMetadata` instance allocated from heap memory. - Call `peekAndCopyMessageMetadata` to cache all message metadata instances in `PersistentDispatcherMultipleConsumers`. It's hard to add tests. As I've explained before, apache#15967 only degrades the performance and doesn't affect the correctness. (cherry picked from commit 36690f5)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Motivation
#7266 introduced the
EntryWrapperto store the
Entryobject and the associatedMessageMetadataif itexists. However, the
getEntryfield ofEntryWrapperis never used.There is no need to allocate memory for
EntryWrapper, even if it'sallocated from the recycler pool.
Modifications
EntryWrapperobject, just iterate over the parsed message metadata list.
MessageMetadataarray tofilterEntriesForConsumerand add the JavaDocs for these two parameters.
After that, Remove unused
EntryWrapperandupdateEntryWrapperWithMetadata.This PR uses functional programming style to make code more simple and clear.
Verifying this change
This change is already covered by existing tests, such as
testBatchMessageDispatchingAccordingToPermits.Documentation
Check the box below or label this PR directly.
Need to update docs?
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)