[feat][client]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend #17195

Jason918 · 2022-08-21T11:12:41Z

Master Issue: #16153

Motivation

See PIP-180 #16153. Shadow topic requires message id from shadow replicator to update LAC and populate entry cache.

Modifications

Add message_id in CommandSend.

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

ProduceWithMessageIdTest

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API: (yes)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

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

This new field is for internal use only right now.

doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

Jason918 · 2022-08-21T12:43:07Z

/pulsarbot run-failure-checks

eolivelli · 2022-08-22T18:32:55Z

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java

+    protected ByteBufPair sendMessage(long producerId, long sequenceId, int numMessages,
+                                      MessageId messageId, MessageMetadata msgMetadata,
+                                      ByteBuf compressedPayload) {
+        if (messageId instanceof MessageIdImpl) {


This shouldn't be a code path taken by user Pulsar clients but only by the Replicator.

Maybe we should add an internal flag to the PulsarClient to mark it as a internal client and allow it to be able to do this thing.
Pushing the messageId from the producer also may lead to some security issues.
We must prevent on the broker side that a client writes to the wrong ledger (for instance to a ledger of another topic, that can be not accessible from the client according the authz)

So we have two problems:

regular users should not be able to set the MessageId (I don't know if there is a way)

the broker must not allow a client to write to a ledger that is not part of the topic

@eolivelli

regular users should not be able to set the MessageId (I don't know if there is a way)

For normal users, the public method TypedMessageBuilder<T> Producer#newMessage() should be used to construct and send messages, but they can't set MessageId in TypedMessageBuilder. So messageId won't be set unless users know what they are doing.
And currently, messageId only can be send to broker with public methods in ProducerImpl. It's already kind of an internal usage.

the broker must not allow a client to write to a ledger that is not part of the topic

Sure, I understand your concern, but it won't happen. The messageId will only be used by ShadowTopic and they won't have any write access to these ledgers or any ledgers. For normal topic, in case they received this messageId by some accident, they will just ignore it.

@eolivelli PTAL

It's very confusing to see the logic here. What's the behavior if the ledger id and the entry id are both -1 in a CommandSend? Is this else branch protected by unit tests? @Jason918 @eolivelli

eolivelli

I approve the patch.
But when we go down the implementation I would like to see some check about the "ledgerId".
We already had in the past problems with clients that are able to set a ledgerId and that caused lot of troubles (CVEs)

Jason918 · 2022-08-26T10:55:42Z

I approve the patch. But when we go down the implementation I would like to see some check about the "ledgerId". We already had in the past problems with clients that are able to set a ledgerId and that caused lot of troubles (CVEs)

Sure

…and apache#16605 - apache#17195 changed the method signature that apache#16605 depended upon

…16605 (#17300) - #17195 changed the method signature that #16605 depended upon

…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

…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)

Add messgae_id in CommandSend for Shadow Topic

7823cb2

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 21, 2022

Jason918 self-assigned this Aug 21, 2022

Jason918 added this to the 2.12.0 milestone Aug 21, 2022

Jason918 added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Aug 21, 2022

Jason918 requested review from AnonHxy, codelipenghui, hangc0276, Technoboy-, lhotari and eolivelli August 21, 2022 13:47

codelipenghui approved these changes Aug 22, 2022

View reviewed changes

eolivelli requested changes Aug 22, 2022

View reviewed changes

Jason918 changed the title ~~[PIP-180] ShadowTopic - Part II - Add support for messgae_id in CommandSend~~ [feat][broker]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend Aug 23, 2022

Jason918 changed the title ~~[feat][broker]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend~~ [feat][client]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend Aug 23, 2022

Jason918 requested a review from eolivelli August 24, 2022 05:54

eolivelli approved these changes Aug 26, 2022

View reviewed changes

eolivelli merged commit 6798f88 into apache:master Aug 26, 2022

lhotari added a commit to lhotari/pulsar that referenced this pull request Aug 26, 2022

[fix][broker] Fix broken build caused by conflict between apache#17195 …

6b8c6a3

…and apache#16605 - apache#17195 changed the method signature that apache#16605 depended upon

This was referenced Aug 26, 2022

[fix][broker] Fix broken build caused by conflict between #17195 and #16605 #17300

Merged

[fix][client]sendMessage param miss in createOpSendMsg cause CI Build Error #17297

Closed

lhotari added a commit that referenced this pull request Aug 26, 2022

[fix][broker] Fix broken build caused by conflict between #17195 and #…

ba9c01f

…16605 (#17300) - #17195 changed the method signature that #16605 depended upon

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Aug 29, 2022

Add messgae_id in CommandSend for Shadow Topic (apache#17195)

722868a

codelipenghui mentioned this pull request Aug 31, 2022

[improve][replicator] add the original message ID to the replicated cluster's messages #17356

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat][client]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend #17195

[feat][client]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend #17195

Jason918 commented Aug 21, 2022 •

edited

Loading

Jason918 commented Aug 21, 2022

eolivelli Aug 22, 2022

eolivelli Aug 22, 2022

Jason918 Aug 23, 2022

Jason918 Aug 26, 2022

BewareMyPower Dec 8, 2022

eolivelli left a comment

Jason918 commented Aug 26, 2022

[feat][client]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend #17195

[feat][client]PIP-180 ShadowTopic - Part II - Add support for messgae_id in CommandSend #17195

Conversation

Jason918 commented Aug 21, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Jason918 commented Aug 21, 2022

eolivelli Aug 22, 2022

Choose a reason for hiding this comment

eolivelli Aug 22, 2022

Choose a reason for hiding this comment

Jason918 Aug 23, 2022

Choose a reason for hiding this comment

Jason918 Aug 26, 2022

Choose a reason for hiding this comment

BewareMyPower Dec 8, 2022

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

Jason918 commented Aug 26, 2022

Jason918 commented Aug 21, 2022 •

edited

Loading