[fix][monitor] Fix the partitioned publisher topic stat aggregation bug #18807

heesung-sn · 2022-12-07T17:49:07Z

Motivation

The index-based publisher stat aggregation(configured by aggregatePublisherStatsByProducerName=false, default, triggered by pulsar-admin topics partitioned-stats api) can burst memory and wrongly aggregate publisher metrics if each partition stat returns a different size or order of the publisher stat list.
In the worst case, if there are many partitions and publishers created and closed concurrently, the current code can create PublisherStatsImpl objects exponentially, and this can cause a high GC time or OOM.

Discussion Thread:

https://lists.apache.org/thread/vofv1oz0wvzlwk4x9vk067rhkscn8bqo

Issue Code reference:

2c428f7#diff-02e50674125a597f8ae3405a884590759f2fdaa10104cea511d5ea44b6ff6490R224-R247

Modifications

Fixed the bug code (the nested loop) that could exponentially create PublisherStatsImpl objects.

Verifying this change

Make sure that the change passes the CI checks.

This change added unit tests.

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: heesung-sn#15

codecov-commenter · 2022-12-11T02:04:14Z

Codecov Report

Merging #18807 (f7e7f12) into master (492a9c3) will increase coverage by 1.14%.
The diff coverage is 28.57%.

@@             Coverage Diff              @@
##             master   #18807      +/-   ##
============================================
+ Coverage     47.46%   48.60%   +1.14%     
- Complexity    10727    11308     +581     
============================================
  Files           711      712       +1     
  Lines         69456    72833    +3377     
  Branches       7452     8303     +851     
============================================
+ Hits          32964    35404    +2440     
- Misses        32810    33577     +767     
- Partials       3682     3852     +170

Flag	Coverage Δ
unittests	`48.60% <28.57%> (+1.14%)`	⬆️

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

Impacted Files	Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java	`15.06% <0.00%> (-0.04%)`	⬇️
.../pulsar/broker/admin/impl/SchemasResourceBase.java	`90.09% <100.00%> (+0.20%)`	⬆️
...lsar/client/impl/conf/ClientConfigurationData.java	`96.66% <100.00%> (ø)`
...java/org/apache/pulsar/proxy/stats/TopicStats.java	`58.82% <0.00%> (-41.18%)`	⬇️
...a/org/apache/bookkeeper/mledger/ManagedCursor.java	`50.00% <0.00%> (-35.72%)`	⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java	`54.12% <0.00%> (-13.77%)`	⬇️
...ava/org/apache/pulsar/broker/service/Consumer.java	`68.86% <0.00%> (-4.10%)`	⬇️
...roker/service/persistent/MessageDeduplication.java	`50.65% <0.00%> (-3.06%)`	⬇️
...rg/apache/pulsar/broker/service/AbstractTopic.java	`62.06% <0.00%> (-3.00%)`	⬇️
...pache/pulsar/broker/admin/v2/PersistentTopics.java	`72.47% <0.00%> (-1.94%)`	⬇️
... and 53 more

equanz · 2022-12-15T10:35:19Z

pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/TopicStatsImpl.java

+               if (index == this.publishers.size()) {
+                   PublisherStatsImpl newStats = new PublisherStatsImpl();
+                   newStats.setSupportsPartialProducer(false);
+                   this.publishers.add(newStats);
               }
+               this.publishers.get(index)
+                       .add((PublisherStatsImpl) s);


nits:

The publishers field is not thread-safe from before. Potentially access to it from concurrent threads.
Moreover, it doesn't guarantee the size is index + 1 in L262 because it isn't held mutual lock between L259 and L262.

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/TopicStatsImpl.java

Line 111 in 1107af2

private List<PublisherStatsImpl> publishers;

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/TopicStatsImpl.java

Line 179 in 1107af2

this.publishers = new ArrayList<>();

We want to avoid IndexOutOfBoundsException completely.

Hi,

I see that *TopicStatsImpl is not a thread-safe class. Currently, the caller needs to externally synchronize the access if concurrent threads access the same instance.

Potentially access to it from concurrent threads.

Could you provide this example? I see that PartitionedTopicStatsImpl is instantiated by a single thread, and add(..) gets called from the same thread.

PartitionedTopicStatsImpl stats = new PartitionedTopicStatsImpl(partitionMetadata); ... FutureUtil.waitForAll(topicStatsFutureList).handle((result, exception) -> { ... stats.add(statFuture.get()); if (perPartition) { stats.getPartitions().put(topicName.getPartition(i).toString(), statFuture.get()); ... } }

This case may not be in the current codes. If contributors already know "the caller needs to externally synchronize the access," we don't care in this section. I'm worried about misimplementation. So, I think it's okay to write comments about the current spec instead of fixing it to follow my comment.

Hi, sorry. Do you mean we need to comment that PartitionedTopicStatsImpl is not thread-safe?

Otherwise, PTAL again (or could you approve this?)

Do you mean we need to comment that PartitionedTopicStatsImpl is not thread-safe?

Yes.

Added comments.

heesung-sn · 2022-12-28T23:30:28Z

@eolivelli could you review this PR by any chance?

codelipenghui

Is it better to split it into 2 PRs? One is to fix the aggregation issue, another change the default value.

So that we can ship the fix to release branches, and the default configuration change only applies to the master branch (release in the next major version).

codelipenghui · 2022-12-30T05:34:05Z

.../src/main/java/org/apache/pulsar/common/policies/data/stats/NonPersistentTopicStatsImpl.java

+                if (index == this.nonPersistentPublishers.size()) {
+                    NonPersistentPublisherStatsImpl newStats = new NonPersistentPublisherStatsImpl();
+                    newStats.setSupportsPartialProducer(false);
+                    this.nonPersistentPublishers.add(newStats);


It's better to add some description for this if branch. Like why we need to use a newStats if index == this.nonPersistentPublishers.size(). And what does index == this.nonPersistentPublishers.size() exactly means. Frankly, it's not easy to understand. I think if we have some description, it will help the readers.

Added comments.

…ult to true" This reverts commit bc37696.

heesung-sn · 2022-12-30T19:22:16Z

Is it better to split it into 2 PRs? One is to fix the aggregation issue, another change the default value.

So that we can ship the fix to release branches, and the default configuration change only applies to the master branch (release in the next major version).

I reverted the config default change in this PR.
I raised another PR for the config default change here. #19114

poorbarcode · 2023-05-07T16:19:14Z

@heesung-sn

I have added the label [release/2.10.5, release/2.11.2] and cherry-picked this patch into these branches.

…ug (#18807) (cherry picked from commit 8790ed1)

…ug (apache#18807) (cherry picked from commit 8790ed1) (cherry picked from commit b5b2de6)

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

Demogorgon314 assigned heesung-sn Dec 8, 2022

Demogorgon314 added type/bug The PR fixed a bug or issue reported a bug area/metrics labels Dec 8, 2022

Technoboy- added this to the 2.12.0 milestone Dec 11, 2022

Technoboy- added the ready-to-test label Dec 11, 2022

Technoboy- closed this Dec 11, 2022

Technoboy- reopened this Dec 11, 2022

Technoboy- changed the title ~~[fix][metrics] fixed a partitioned publisher topic stat aggregation bug~~ [fix][monitor] Fix a partitioned publisher topic stat aggregation bug Dec 11, 2022

nkurihar approved these changes Dec 15, 2022

View reviewed changes

equanz reviewed Dec 15, 2022

View reviewed changes

heesung-sn added 4 commits December 28, 2022 17:18

fixed the nested partitioned publisher stat aggregation bug

5d5e741

Changed the aggregatePublisherStatsByProducerName config default to true

bc37696

use simple for loop

8154fce

Added not thread-safe comments

7a7aaec

heesung-sn force-pushed the bug-fix-topic-stats-nested branch from 7be4cdd to 7a7aaec Compare December 29, 2022 01:42

equanz approved these changes Dec 29, 2022

View reviewed changes

codelipenghui reviewed Dec 30, 2022

View reviewed changes

heesung-sn added 2 commits December 30, 2022 11:01

added more comments

c416691

Revert "Changed the aggregatePublisherStatsByProducerName config defa…

f7e7f12

…ult to true" This reverts commit bc37696.

codelipenghui approved these changes Jan 3, 2023

View reviewed changes

heesung-sn changed the title ~~[fix][monitor] Fix a partitioned publisher topic stat aggregation bug~~ [fix][monitor] Fix the partitioned publisher topic stat aggregation bug Jan 3, 2023

codelipenghui closed this Jan 4, 2023

codelipenghui reopened this Jan 4, 2023

codelipenghui merged commit 8790ed1 into apache:master Jan 4, 2023

poorbarcode added release/2.10.5 release/2.11.2 labels May 7, 2023

poorbarcode pushed a commit that referenced this pull request May 7, 2023

[fix][monitor] Fix the partitioned publisher topic stat aggregation b…

b5b2de6

…ug (#18807) (cherry picked from commit 8790ed1)

poorbarcode pushed a commit that referenced this pull request May 7, 2023

[fix][monitor] Fix the partitioned publisher topic stat aggregation b…

a764bbd

…ug (#18807) (cherry picked from commit 8790ed1)

poorbarcode added cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels May 7, 2023

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 11, 2023

[fix][monitor] Fix the partitioned publisher topic stat aggregation b…

8de3ba4

…ug (apache#18807) (cherry picked from commit 8790ed1) (cherry picked from commit b5b2de6)

heesung-sn deleted the bug-fix-topic-stats-nested branch April 2, 2024 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][monitor] Fix the partitioned publisher topic stat aggregation bug #18807

[fix][monitor] Fix the partitioned publisher topic stat aggregation bug #18807

heesung-sn commented Dec 7, 2022 •

edited

Loading

codecov-commenter commented Dec 11, 2022 •

edited

Loading

equanz Dec 15, 2022

heesung-sn Dec 15, 2022

equanz Dec 17, 2022

heesung-sn Dec 28, 2022 •

edited

Loading

equanz Dec 28, 2022

heesung-sn Dec 29, 2022

heesung-sn commented Dec 28, 2022

codelipenghui left a comment

codelipenghui Dec 30, 2022

heesung-sn Dec 30, 2022

heesung-sn commented Dec 30, 2022

poorbarcode commented May 7, 2023 •

edited

Loading

[fix][monitor] Fix the partitioned publisher topic stat aggregation bug #18807

[fix][monitor] Fix the partitioned publisher topic stat aggregation bug #18807

Conversation

heesung-sn commented Dec 7, 2022 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

codecov-commenter commented Dec 11, 2022 • edited Loading

Codecov Report

equanz Dec 15, 2022

Choose a reason for hiding this comment

heesung-sn Dec 15, 2022

Choose a reason for hiding this comment

equanz Dec 17, 2022

Choose a reason for hiding this comment

heesung-sn Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

equanz Dec 28, 2022

Choose a reason for hiding this comment

heesung-sn Dec 29, 2022

Choose a reason for hiding this comment

heesung-sn commented Dec 28, 2022

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Dec 30, 2022

Choose a reason for hiding this comment

heesung-sn Dec 30, 2022

Choose a reason for hiding this comment

heesung-sn commented Dec 30, 2022

poorbarcode commented May 7, 2023 • edited Loading

heesung-sn commented Dec 7, 2022 •

edited

Loading

codecov-commenter commented Dec 11, 2022 •

edited

Loading

heesung-sn Dec 28, 2022 •

edited

Loading

poorbarcode commented May 7, 2023 •

edited

Loading