Skip to content
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

[fix][client] Fix MessageIdUtils cannot handle TopicMessageId #22698

Closed
wants to merge 1 commit into from

Conversation

RobertIndie
Copy link
Member

Motivation

PIP-229 introduces an interface change for the messageId which break the behavior in and after Pulsar 3.0.

This leads to the MessageIdUtils not be able to handle the TopicMessageId. It will throw error like:

java.lang.ClassCastException: class org.apache.pulsar.client.impl.TopicMessageIdImpl cannot be cast to class org.apache.pulsar.client.impl.MessageIdImpl (org.apache.pulsar.client.impl.TopicMessageIdImpl and org.apache.pulsar.client.impl.MessageIdImpl are in unnamed module of loader 'app')

	at org.apache.pulsar.client.util.MessageIdUtils.getOffset(MessageIdUtils.java:27)
...

Modifications

  • Use MessageIdAdv to convert the MessageId to offset

Verifying this change

This change added tests.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@RobertIndie RobertIndie self-assigned this May 11, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 11, 2024
@RobertIndie RobertIndie added type/bug The PR fixed a bug or issue reported a bug ready-to-test release/3.2.3 release/3.0.5 area/client and removed doc-not-needed Your PR changes do not impact docs labels May 11, 2024
@RobertIndie RobertIndie added this to the 3.4.0 milestone May 11, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 11, 2024
@@ -19,11 +19,12 @@
package org.apache.pulsar.client.util;

import org.apache.pulsar.client.api.MessageId;
import org.apache.pulsar.client.api.MessageIdAdv;
import org.apache.pulsar.client.impl.MessageIdImpl;

public class MessageIdUtils {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This util class was introduced at very early era. It's better to add the util function to MessageIdAdvUtils for MessageIdAdv.

And I don't think it's worth providing a util function for MessageIdAdv in the Pulsar core repo. MessageIdAdv is introduced mainly for this reason. The downstream application should cast MessageId to MessageIdAdv to access the internal fields. It should not depend on the wrapped APIs from the upstream.

Pulsar does not have the "offset" concept, the same logic is only used in the Kafka sink connector, but it does not call this util method directly. See

To get the offset, it's better to leverage the AppendIndexMetadataInterceptor like KoP. See the discussion here: streamnative/kop#290

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also used in third-party connectors, not just limited to the kafka sink connector.
Removing the offset from the pulsar-client looks good to me. I think we can mark it with Deprecate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Before removing it, we can mark it with @Deprecated.

In 3rd party applications, it's easy to customize its own implementation via MessageIdAdv. You can see the example from an early implementation of KoP: https://github.com/streamnative/kop/blob/branch-2.7.4.5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/utils/MessageIdUtils.java, which is also an example to show the way to compute the offset is not standard.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To leave our discussion context here, I created a new PR to deprecate these APIs: #22747.
There is the same offset implementation in the FunctionCommon. I don't deprecate them currently because the connector still needs to use it.

RobertIndie added a commit that referenced this pull request May 21, 2024
…Utils.getMessageId` (#22747)

### Motivation

After discussing [here](#22698 (comment)),  the pulsar client shouldn't expose the `offset` term to users.

### Modifications

- Deprecate `MessageIdUtils.getOffset` and `MessageIdUtils.getMessageId`
- For connectors, use `FunctionCommon.getOffset` and `FunctionCommon.getMessageId`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/client doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.5 release/3.2.3 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants