[improve][client] Support multi-topic messageId deserialization to ack messages #19944

rdhabalia · 2023-03-28T01:12:23Z

Motivation

Right now, when user tries to serialize messageId and deserialize messageId for a partitioned-topic and lacks the message then it fails with the below exception.

org.apache.pulsar.client.api.PulsarClientException$NotAllowedException: Only TopicMessageId is allowed to acknowledge for a multi-topics consumer, while messageId is org.apache.pulsar.client.impl.BatchMessageIdImpl
	at org.apache.pulsar.client.api.PulsarClientException.unwrap(PulsarClientException.java:1083)
	at org.apache.pulsar.client.impl.ConsumerBase.acknowledge(ConsumerBase.java:418)
	at org.apache.pulsar.client.api.MultiTopicsConsumerTest.testMultiTopicAckWithByteMessageId(MultiTopicsConsumerTest.java:409)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

It happens because MessageId is not able deserialize into TopicMessageIdImpl and because of that MultiTopicsConsumerImpl fails to ack the message.

Modifications

Support serializing and deserialzing for messageId(TopicMessageIdImpl) of MultiTopicsConsumer.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

BewareMyPower · 2023-03-31T02:56:02Z

pulsar-common/src/main/proto/PulsarApi.proto

    // For the chunk message id, we need to specify the first chunk message id.
    optional MessageIdData first_chunk_message_id = 7;
+    optional string topicName = 8;


I think we need a PIP to add new fields to the Pulsar API. In PIP-224 I proposed to add an extra class TopicMessageIdSerDes for the serialization of the TopicMessageId, but the related PR is not present yet. If we can introduce such a new field, I think we don't need to implement TopicMessageIdSerDes any more.

As I described here, the limit of PIP-224 is caused by the lack of the topic name field in MessageIdData. I think it would be better to add this field before the 3.0.0 release. But I'd like to hear more voices about this change.

@BewareMyPower I replied on the PR serializing and deserializing is expensive and on top of that having different APIs for different use cases is creating a really bad experience for users, and I strongly feel we should avoid such APIs and complexity if things can be solved with a simple straight forward change with the same API and without creating the bad user experience.
I think we should consider this simple change without costing performance and API incompatibility and confusing usage to the users. So, I would like to avoid this change: BewareMyPower#20

Actually I'm +1 to your PR and I think the API proposed in PIP-224 could be replaced with this PR.

few committers block all the PRs without any reason

However, the reason I blocked this PR is the current requirement of the PIP:

Any change to the wire protocol APIs

This PR brings a change to the wire protocol API, and I think it should not be passed without any proposal. /cc @merlimat @eolivelli @codelipenghui

Hmmm, I think it's better to add a description to clarify the new field will not apply to RPC calls. It is only used by the client side to carry the topic name.

BewareMyPower · 2023-04-04T09:32:41Z

Could you resolve the conflicts with master?

lifepuzzlefun · 2023-04-08T06:31:02Z

pulsar-common/src/main/proto/PulsarApi.proto

    // For the chunk message id, we need to specify the first chunk message id.
    optional MessageIdData first_chunk_message_id = 7;
+    optional string topicName = 8;


hi, i come from Kafka. and i think when client enable group commit offset. the field can be group by topicName and we can have much smaller RPC message to send on the wire. see kafka OffsetCommit RPC https://kafka.apache.org/protocol.html#The_Messages_OffsetCommit : - )

poorbarcode · 2023-04-10T17:02:43Z

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

The PR of type feature is deferred to 3.1.0
The PR of type fix is deferred to 3.0.1

So drag this PR to 3.1.0

rdhabalia · 2023-04-11T21:52:44Z

@BewareMyPower anyways, I have resolved the conflict to move this PR forward.

…alization to ack messages

codecov-commenter · 2023-04-12T02:27:05Z

Codecov Report

Merging #19944 (04f52fa) into master (dc1cdac) will increase coverage by 48.66%.
The diff coverage is 61.11%.

@@              Coverage Diff              @@
##             master   #19944       +/-   ##
=============================================
+ Coverage     24.26%   72.92%   +48.66%     
- Complexity      294    31857    +31563     
=============================================
  Files          1609     1865      +256     
  Lines        125669   138191    +12522     
  Branches      13707    15204     +1497     
=============================================
+ Hits          30490   100779    +70289     
+ Misses        90689    29393    -61296     
- Partials       4490     8019     +3529

Flag	Coverage Δ
inttests	`24.19% <18.05%> (-0.07%)`	⬇️
systests	`25.00% <29.16%> (?)`
unittests	`72.18% <61.11%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...sar/broker/authorization/AuthorizationService.java	`58.00% <ø> (+52.79%)`	⬆️
.../pulsar/broker/service/BrokerServiceException.java	`86.84% <0.00%> (+65.93%)`	⬆️
.../apache/pulsar/client/impl/BatchMessageIdImpl.java	`82.75% <0.00%> (+18.47%)`	⬆️
...org/apache/pulsar/proxy/server/ProxyClientCnx.java	`79.41% <ø> (+79.41%)`	⬆️
...rg/apache/pulsar/proxy/server/ProxyConnection.java	`58.76% <33.33%> (+22.30%)`	⬆️
...sar/broker/service/persistent/PersistentTopic.java	`79.34% <40.00%> (+33.23%)`	⬆️
...n/java/org/apache/pulsar/broker/PulsarService.java	`81.63% <50.00%> (+27.98%)`	⬆️
.../apache/pulsar/client/impl/TopicMessageIdImpl.java	`58.33% <50.00%> (-4.17%)`	⬇️
...a/org/apache/pulsar/client/impl/MessageIdImpl.java	`89.87% <87.50%> (+53.76%)`	⬆️
...va/org/apache/pulsar/broker/service/ServerCnx.java	`71.37% <90.00%> (+40.70%)`	⬆️
... and 3 more

... and 1574 files with indirect coverage changes

Changes are done

codelipenghui

It doesn't look to me to introduce wire protocol changes that without PIP.

codelipenghui · 2023-04-19T12:01:23Z

where did we discuss this? do you want me to show examples where breaking changes and critical impact PRs were merged which should not be merged. Sorry, I can't keep spending time in rebasing PR.

@rdhabalia I mean it should start with a PIP because the PR introduced changes to the wire protocol. If the PR fixes a breaking change, why the PR title is using [improve], it confused me. Starting with a proposal will help us understand the whole context of the issue and why this solution is the best solution for the problem.

rdhabalia · 2023-05-04T01:05:31Z

@codelipenghui
I have created PIP: #20221

BewareMyPower · 2023-05-05T03:52:03Z

You need to send a discussion to [email protected], otherwise others might not know this PIP. For example, I see the PIP number of @liangyepianzhou has conflict with this PIP. #20225

Reference:

The PIP workflow: https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md#how-does-the-pip-process-work

liangyepianzhou · 2023-05-05T04:04:34Z

You need to send a discussion to [email protected], otherwise others might not know this PIP. For example, I see the PIP number of @liangyepianzhou has conflict with this PIP. #20225

I recall it was mentioned before that we should select the next PIP number based on the PIP numbers in the emails. Before sending the vote email, I searched and found that PIP-266 had not been used.

rdhabalia · 2023-05-09T05:34:07Z

@liangyepianzhou @BewareMyPower
It seems this PIP was created before #20225 . But anyways, I will rename my PIP to 267 to resolve the conflict.

github-actions · 2023-06-09T02:06:52Z

The pr had no activity for 30 days, mark with Stale label.

eolivelli

+1

eolivelli · 2023-06-09T08:10:28Z

I had missed this work.
I support it

Dismiss the review

github-actions · 2023-07-22T01:55:07Z

The pr had no activity for 30 days, mark with Stale label.

rdhabalia added the area/client label Mar 28, 2023

rdhabalia added this to the 3.0.0 milestone Mar 28, 2023

rdhabalia self-assigned this Mar 28, 2023

rdhabalia added doc-not-needed Your PR changes do not impact docs ready-to-test labels Mar 28, 2023

rdhabalia changed the title ~~[improve][client] Pulsar client supports multi-topic messageId deserialization to ack messages~~ [improve][client] Support multi-topic messageId deserialization to ack messages Mar 28, 2023

BewareMyPower previously requested changes Mar 31, 2023

View reviewed changes

rdhabalia mentioned this pull request Apr 3, 2023

PIP-224: Introduce TopicMessageId for consumer's MessageId related APIs #18616

Open

BewareMyPower requested review from merlimat, Technoboy-, eolivelli, 315157973, codelipenghui and mattisonchao April 4, 2023 07:55

lifepuzzlefun reviewed Apr 8, 2023

View reviewed changes

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

rdhabalia force-pushed the mid_ser branch from 8f2183e to 1c2d181 Compare April 11, 2023 21:51

[improve][client] Pulsar client supports multi-topic messageId deseri…

04f52fa

…alization to ack messages

rdhabalia force-pushed the mid_ser branch from 1c2d181 to 04f52fa Compare April 11, 2023 22:35

codelipenghui previously requested changes Apr 12, 2023

View reviewed changes

rdhabalia mentioned this pull request May 4, 2023

PIP-267: Support multi-topic messageId deserialization to ack messages #20221

Open

github-actions bot added the Stale label Jun 9, 2023

eolivelli approved these changes Jun 9, 2023

View reviewed changes

github-actions bot removed the Stale label Jun 10, 2023

github-actions bot added the Stale label Jul 22, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

lhotari added the release/4.0.1 label Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][client] Support multi-topic messageId deserialization to ack messages #19944

[improve][client] Support multi-topic messageId deserialization to ack messages #19944

rdhabalia commented Mar 28, 2023 •

edited

Loading

BewareMyPower Mar 31, 2023

BewareMyPower Mar 31, 2023

rdhabalia Apr 3, 2023

BewareMyPower Apr 4, 2023 •

edited

Loading

codelipenghui Apr 19, 2023

BewareMyPower commented Apr 4, 2023

lifepuzzlefun Apr 8, 2023

poorbarcode commented Apr 10, 2023

rdhabalia commented Apr 11, 2023

codecov-commenter commented Apr 12, 2023

codelipenghui left a comment

codelipenghui commented Apr 19, 2023

rdhabalia commented May 4, 2023

BewareMyPower commented May 5, 2023

liangyepianzhou commented May 5, 2023

rdhabalia commented May 9, 2023

github-actions bot commented Jun 9, 2023

eolivelli left a comment

eolivelli commented Jun 9, 2023

github-actions bot commented Jul 22, 2023

[improve][client] Support multi-topic messageId deserialization to ack messages #19944

Are you sure you want to change the base?

[improve][client] Support multi-topic messageId deserialization to ack messages #19944

Conversation

rdhabalia commented Mar 28, 2023 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

BewareMyPower Mar 31, 2023

Choose a reason for hiding this comment

BewareMyPower Mar 31, 2023

Choose a reason for hiding this comment

rdhabalia Apr 3, 2023

Choose a reason for hiding this comment

BewareMyPower Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

codelipenghui Apr 19, 2023

Choose a reason for hiding this comment

BewareMyPower commented Apr 4, 2023

lifepuzzlefun Apr 8, 2023

Choose a reason for hiding this comment

poorbarcode commented Apr 10, 2023

rdhabalia commented Apr 11, 2023

codecov-commenter commented Apr 12, 2023

Codecov Report

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui commented Apr 19, 2023

rdhabalia commented May 4, 2023

BewareMyPower commented May 5, 2023

liangyepianzhou commented May 5, 2023

rdhabalia commented May 9, 2023

github-actions bot commented Jun 9, 2023

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented Jun 9, 2023

github-actions bot commented Jul 22, 2023

rdhabalia commented Mar 28, 2023 •

edited

Loading

BewareMyPower Apr 4, 2023 •

edited

Loading