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][misc] Topic name from persistence name should decode local name #22879

Conversation

Shawyeok
Copy link
Contributor

@Shawyeok Shawyeok commented Jun 7, 2024

Motivation

The methods TopicName#getPersistenceNamingEncoding and TopicName#fromPersistenceNamingEncoding should be symmetric. Currently, the localName part of the topic name is encoded into the persistence name, but it is not decoded back properly.

Modifications

This update add decoding logic for the localName part of the topic name.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • A new test case has been added to org.apache.pulsar.common.naming.TopicNameTest#testFromPersistenceNamingEncoding that includes topic names with special characters.

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: Shawyeok#16

@Shawyeok Shawyeok changed the title [fix][misc] Topic name from persistence naming encoding should decode local name [fix][misc] Topic name from persistence name should decode local name Jun 7, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 7, 2024
@Shawyeok Shawyeok force-pushed the fix/topic-name-from-persistence-naming-encoding branch from ffb901b to ce65db9 Compare June 7, 2024 17:37
@lhotari
Copy link
Member

lhotari commented Jun 7, 2024

@Shawyeok Just curious to know what the impact of the previous behavior was. Did it cause actual problems? Any error messages in logs?

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@Shawyeok
Copy link
Contributor Author

Shawyeok commented Jun 8, 2024

@Shawyeok Just curious to know what the impact of the previous behavior was. Did it cause actual problems? Any error messages in logs?

The method TopicName#fromPersistenceNamingEncoding is utilized in my downstream application to monitor the ZooKeeper paths /admin/partitioned-topics and /managed-ledger. It listens for the creation of new topics of interest and changes to topic partitions.

Moreover reviewing references in the Pulsar codebase, I noticed that this may lead to inaccuracies in the topicName label of some offloader metrics.

String topicName = TopicName.fromPersistenceNamingEncoding(managedLedgerName);

Copy link
Contributor

@hanmz hanmz left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari added this to the 3.4.0 milestone Jun 11, 2024
@lhotari lhotari merged commit c326d8e into apache:master Jun 11, 2024
53 of 55 checks passed
lhotari pushed a commit that referenced this pull request Jun 13, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 17, 2024
lhotari pushed a commit that referenced this pull request Jun 19, 2024
lhotari pushed a commit that referenced this pull request Jun 19, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 25, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants