Skip to content

[fix][broker] Fix to avoid TopicStatsImpl NPE even if producerName is null#15502

Merged
Technoboy- merged 2 commits into
apache:masterfrom
equanz:fix_to_avoid_topicstatsimpl_npe
May 14, 2022
Merged

[fix][broker] Fix to avoid TopicStatsImpl NPE even if producerName is null#15502
Technoboy- merged 2 commits into
apache:masterfrom
equanz:fix_to_avoid_topicstatsimpl_npe

Conversation

@equanz

@equanz equanz commented May 9, 2022

Copy link
Copy Markdown
Contributor

Fixes #15471

Motivation

Please see #15471 .

Modifications

  • if producerName is null, then fall back to supportsPartialProducer=false
    • currently, producerName is required whensupportsPartialProducer=true, because stats are aggregated by producerName
  • allow null producerName on getPublishers

Verifying this change

  • Make sure that the change passes the CI checks.
  • Add tests for null producerName

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): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • no-need-doc

This PR is one of the fixing issues.

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 9, 2022
@Technoboy-

Copy link
Copy Markdown
Contributor

Why the produceName is null?

@Technoboy-

Copy link
Copy Markdown
Contributor

Why the produceName is null?

Get it from the issue comment.

@Technoboy-

Copy link
Copy Markdown
Contributor

I think the root cause is here :

if (this.publishers.size() != stats.publishers.size()) {
for (int i = 0; i < stats.publishers.size(); i++) {
PublisherStatsImpl newStats = new PublisherStatsImpl();
newStats.setSupportsPartialProducer(false);
this.publishers.add(newStats.add(stats.publishers.get(i)));

We should set producerName for the newStats

@equanz

equanz commented May 12, 2022

Copy link
Copy Markdown
Contributor Author

I think the root cause is here :

Yes. I think so too.

We should set producerName for the newStats

I think we can't set producerName in this section, because old client's partitioned producer can have different producerName between internal producers. #12401

@Technoboy-

Copy link
Copy Markdown
Contributor

I think the root cause is here :

Yes. I think so too.

We should set producerName for the newStats

I think we can't set producerName in this section, because old client's partitioned producer can have different producerName between internal producers. #12401

Ok, make sense.

@Technoboy- Technoboy- added type/bug The PR fixed a bug or issue reported a bug area/broker release/2.10.1 labels May 12, 2022
@Technoboy- Technoboy- added this to the 2.11.0 milestone May 12, 2022
@Technoboy- Technoboy- closed this May 14, 2022
@Technoboy- Technoboy- reopened this May 14, 2022
@Technoboy-
Technoboy- merged commit 7bdfa3a into apache:master May 14, 2022
@equanz
equanz deleted the fix_to_avoid_topicstatsimpl_npe branch May 14, 2022 05:50
codelipenghui pushed a commit that referenced this pull request May 20, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 23, 2022
… null (apache#15502)

(cherry picked from commit 7bdfa3a)
(cherry picked from commit d23e251)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1 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.

NPE during get topic stats Update Pulsar to version 2.10.0,bin/pulsar-admin command invalid.

3 participants