[improve][client]PIP-189: No batching if only one message in batch #16605
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
AnonHxy
force-pushed
the
non_batch_one_msg
branch
from
0348e4a
to
97717fd
Compare
AnonHxy
force-pushed
the
non_batch_one_msg
branch
4 times, most recently
from
667710d
to
4e66d8d
Compare
AnonHxy
changed the title
AnonHxy
changed the title
AnonHxy
force-pushed
the
non_batch_one_msg
branch
4 times, most recently
from
559d42d
to
dc2fd44
Compare
|
/pulsarbot run-failure-checks |
AnonHxy
force-pushed
the
non_batch_one_msg
branch
2 times, most recently
from
3ff33cb
to
e119a97
Compare
AnonHxy
changed the title
AnonHxy
force-pushed
the
non_batch_one_msg
branch
2 times, most recently
from
8bdd2c2
to
bb70eb6
Compare
AnonHxy
changed the title
AnonHxy
force-pushed
the
non_batch_one_msg
branch
2 times, most recently
from
a74caf1
to
729553c
Compare
|
/pulsarbot run-failure-checks |
AnonHxy
force-pushed
the
non_batch_one_msg
branch
from
729553c
to
1c74f2a
Compare
eolivelli
requested changes
Jul 27, 2022
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TopicReaderTest.java
Show resolved
Hide resolved
pulsar-client-tools-test/src/test/java/org/apache/pulsar/client/cli/PulsarClientToolTest.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageContainerImpl.java
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerEntryMetadataE2ETest.java
Show resolved
Hide resolved
eolivelli
reviewed
Jul 31, 2022
There was a problem hiding this comment.
You are on your way.
Now the problem is to fix the remaining tests.
I wish to see this in 2.11 if possible, as it is a important behaviour change and it is better to not introduce it in a point release (like 2.11.1)
@codelipenghui @BewareMyPower @lhotari @michaeljmarshall @merlimat @rdhabalia
pulsar-broker/src/test/java/org/apache/pulsar/client/api/TopicReaderTest.java
Show resolved
Hide resolved
pulsar-client-tools/src/main/java/org/apache/pulsar/client/cli/CmdProduce.java
Outdated
Show resolved
Hide resolved
AnonHxy
force-pushed
the
non_batch_one_msg
branch
4 times, most recently
from
577a198
to
228cb2c
Compare
AnonHxy
added
area/broker
release/2.11.1
type/enhancement
lhotari
added a commit
to lhotari/pulsar
that referenced
this pull request
Aug 26, 2022
…and apache#16605 - apache#17195 changed the method signature that apache#16605 depended upon
1 task
Nicklee007
pushed a commit
to Nicklee007/pulsar
that referenced
this pull request
Aug 29, 2022
…pache#16605) [improve][client]PIP-189: No batching if only one message in batch apache#16605 ### Motivation * See apache#16619 ### Modifications * See apache#16619 * Most of the Modifications are relevant to `BatchMessageContainerImpl` * Of course there are some tests about batching need to be modified, because batched producer can also pubulish non-batched messages when this PIP applies. The tests include: * `RGUsageMTAggrWaitForAllMsgsTest` * `BatchMessageTest` * `BrokerEntryMetadataE2ETest` * `ClientDeduplicationTest` * `TopicReaderTest` * `PulsarClientToolTest`
lhotari
added a commit
that referenced
this pull request
Aug 31, 2022
…sors (#17273) * [fix][broker] Fix broken build caused by conflict between #17195 and #16605 - #17195 changed the method signature that #16605 depended upon * [fix][broker] Keep sorted list of cursors ordered by read position of active cursors when cacheEvictionByMarkDeletedPosition=false Fixes #16054 - calculate the sorted list of when a read position gets updated - this resolves #9958 in a proper way - #12045 broke the caching solution as explained in #16054 - remove invalid tests - fix tests - add more tests to handle corner cases * Address review comment * Handle durable & non-durable in the correct way * Fix cache tests since now entries get evicted reactively * Address review comment about method names * Change signature for add method so that position must be passed - this is more consistent with cursorUpdated method where the position is passed * Update javadoc for ManagedCursorContainer * Address review comment * Simplify ManagedCursorContainer * Clarify javadoc * Ensure that cursors are tracked by making sure that initial position isn't null unintentionally * Prevent race in updating activeCursors
5 tasks
Technoboy-
pushed a commit
that referenced
this pull request
Oct 31, 2022
…sors (#17273) * [fix][broker] Fix broken build caused by conflict between #17195 and #16605 - #17195 changed the method signature that #16605 depended upon * [fix][broker] Keep sorted list of cursors ordered by read position of active cursors when cacheEvictionByMarkDeletedPosition=false Fixes #16054 - calculate the sorted list of when a read position gets updated - this resolves #9958 in a proper way - #12045 broke the caching solution as explained in #16054 - remove invalid tests - fix tests - add more tests to handle corner cases * Address review comment * Handle durable & non-durable in the correct way * Fix cache tests since now entries get evicted reactively * Address review comment about method names * Change signature for add method so that position must be passed - this is more consistent with cursorUpdated method where the position is passed * Update javadoc for ManagedCursorContainer * Address review comment * Simplify ManagedCursorContainer * Clarify javadoc * Ensure that cursors are tracked by making sure that initial position isn't null unintentionally * Prevent race in updating activeCursors
congbobo184
pushed a commit
that referenced
this pull request
Nov 17, 2022
…sors (#17273) * [fix][broker] Fix broken build caused by conflict between #17195 and #16605 - #17195 changed the method signature that #16605 depended upon * [fix][broker] Keep sorted list of cursors ordered by read position of active cursors when cacheEvictionByMarkDeletedPosition=false Fixes #16054 - calculate the sorted list of when a read position gets updated - this resolves #9958 in a proper way - #12045 broke the caching solution as explained in #16054 - remove invalid tests - fix tests - add more tests to handle corner cases * Address review comment * Handle durable & non-durable in the correct way * Fix cache tests since now entries get evicted reactively * Address review comment about method names * Change signature for add method so that position must be passed - this is more consistent with cursorUpdated method where the position is passed * Update javadoc for ManagedCursorContainer * Address review comment * Simplify ManagedCursorContainer * Clarify javadoc * Ensure that cursors are tracked by making sure that initial position isn't null unintentionally * Prevent race in updating activeCursors (cherry picked from commit 856ef15)
AnonHxy
added a commit
to AnonHxy/pulsar
that referenced
this pull request
Nov 20, 2022
…pache#16605) [improve][client]PIP-189: No batching if only one message in batch apache#16605 ### Motivation * See apache#16619 ### Modifications * See apache#16619 * Most of the Modifications are relevant to `BatchMessageContainerImpl` * Of course there are some tests about batching need to be modified, because batched producer can also pubulish non-batched messages when this PIP applies. The tests include: * `RGUsageMTAggrWaitForAllMsgsTest` * `BatchMessageTest` * `BrokerEntryMetadataE2ETest` * `ClientDeduplicationTest` * `TopicReaderTest` * `PulsarClientToolTest`
5 tasks
|
move release/2.11.0 label to #18548 |
congbobo184
pushed a commit
that referenced
this pull request
Dec 7, 2022
…sors (#17273) * [fix][broker] Fix broken build caused by conflict between #17195 and #16605 - #17195 changed the method signature that #16605 depended upon * [fix][broker] Keep sorted list of cursors ordered by read position of active cursors when cacheEvictionByMarkDeletedPosition=false Fixes #16054 - calculate the sorted list of when a read position gets updated - this resolves #9958 in a proper way - #12045 broke the caching solution as explained in #16054 - remove invalid tests - fix tests - add more tests to handle corner cases * Address review comment * Handle durable & non-durable in the correct way * Fix cache tests since now entries get evicted reactively * Address review comment about method names * Change signature for add method so that position must be passed - this is more consistent with cursorUpdated method where the position is passed * Update javadoc for ManagedCursorContainer * Address review comment * Simplify ManagedCursorContainer * Clarify javadoc * Ensure that cursors are tracked by making sure that initial position isn't null unintentionally * Prevent race in updating activeCursors (cherry picked from commit 856ef15)
BewareMyPower
added a commit
to BewareMyPower/kop
that referenced
this pull request
Feb 22, 2023
### Motivation apache/pulsar#17696 removes the `powermock-reflect` dependency, which leads to a compilation error in KoP. ### Modifications Upgrade the Pulsar dependency to 2.11.0.0-rc5. Then replace `Whitebox.invokeMethod` with `MethodUtils.invokeMethod`. It also fixes the bug that null values are not handled well for non-batched messages. This bug was exposed because of [PIP-189](apache/pulsar#16605). ### TODO Migrate the bug fix for non-batched messages to branch-2.10.x and branch-2.9.x.
3 tasks
Demogorgon314
pushed a commit
to streamnative/kop
that referenced
this pull request
Feb 22, 2023
### Motivation apache/pulsar#17696 removes the `powermock-reflect` dependency, which leads to a compilation error in KoP. ### Modifications Upgrade the Pulsar dependency to 2.11.0.0-rc5. Then replace `Whitebox.invokeMethod` with `MethodUtils.invokeMethod`. It also fixes the bug that null values are not handled well for non-batched messages. This bug was exposed because of [PIP-189](apache/pulsar#16605).
Demogorgon314
pushed a commit
to streamnative/kop
that referenced
this pull request
Feb 22, 2023
### Motivation apache/pulsar#17696 removes the `powermock-reflect` dependency, which leads to a compilation error in KoP. ### Modifications Upgrade the Pulsar dependency to 2.11.0.0-rc5. Then replace `Whitebox.invokeMethod` with `MethodUtils.invokeMethod`. It also fixes the bug that null values are not handled well for non-batched messages. This bug was exposed because of [PIP-189](apache/pulsar#16605). (cherry picked from commit 88fe870)
eolivelli
pushed a commit
to eolivelli/kop
that referenced
this pull request
Feb 28, 2023
### Motivation apache/pulsar#17696 removes the `powermock-reflect` dependency, which leads to a compilation error in KoP. ### Modifications Upgrade the Pulsar dependency to 2.11.0.0-rc5. Then replace `Whitebox.invokeMethod` with `MethodUtils.invokeMethod`. It also fixes the bug that null values are not handled well for non-batched messages. This bug was exposed because of [PIP-189](apache/pulsar#16605).
3 tasks
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.
email discussion thread: https://lists.apache.org/thread/dbq1lrv03bhtk0lr5nwm5txo9ndjplv0
email VOTE thread: https://lists.apache.org/thread/s0wtqpoynh970xp6y08xhncyg93xqopv
Motivation
Modifications
BatchMessageContainerImplRGUsageMTAggrWaitForAllMsgsTestBatchMessageTestBrokerEntryMetadataE2ETestClientDeduplicationTestTopicReaderTestPulsarClientToolTestVerifying this change
org.apache.pulsar.broker.service.BatchMessageTest#testBatchSendOneMessageDocumentation
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)