Skip to content

Changing the topic creation flow and optimize heartbeat topic not trigger compaction.#14643

Merged
codelipenghui merged 4 commits into
apache:masterfrom
Technoboy-:fix-healthcheck-compaction
Mar 14, 2022
Merged

Changing the topic creation flow and optimize heartbeat topic not trigger compaction.#14643
codelipenghui merged 4 commits into
apache:masterfrom
Technoboy-:fix-healthcheck-compaction

Conversation

@Technoboy-

@Technoboy- Technoboy- commented Mar 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

When create persistent topic, create compaction subscription(line-1395) is before topic initialize(line-1397), it's better to do this after initialize:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

Lines 1391 to 1398 in ad2cc2d

PersistentTopic persistentTopic = isSystemTopic(topic)
? new SystemTopic(topic, ledger, BrokerService.this)
: new PersistentTopic(topic, ledger, BrokerService.this);
CompletableFuture<Void> preCreateSubForCompaction =
persistentTopic.preCreateSubscriptionForCompactionIfNeeded();
CompletableFuture<Void> replicationFuture = persistentTopic
.initialize()
.thenCompose(__ -> persistentTopic.checkReplication());

If we change this part, we can optimize the heartbeat topic not trigger the compaction.
#13611 has made the heartbeat topic as a system topic to avoid deleting by GC. However, system topic is compacted by default. But the heartbeat topic sends msg without a key, so trigger compaction is meaningful. So it's better to skip the heartbeat topic to do the compaction.

Modification

  • Move preCreateSubscriptionForCompactionIfNeeded after initialize.
  • Add heartbeat topic not trigger compaction.

Documentation

  • no-need-doc

@Technoboy- Technoboy- self-assigned this Mar 10, 2022
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Mar 10, 2022
@codelipenghui codelipenghui modified the milestones: 2.10.0, 2.11.0 Mar 10, 2022
codelipenghui
codelipenghui reviewed Mar 10, 2022
View reviewed changes
Comment thread pulsar-broker/src/test/java/org/apache/pulsar/compaction/CompactionTest.java Outdated
Technoboy-
Technoboy- commented Mar 12, 2022
View reviewed changes
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
.thenCompose(__ -> persistentTopic.checkReplication())
.thenCompose(v -> {
// Also check dedup status
return persistentTopic.checkDeduplicationStatus();

@Technoboy- Technoboy- Mar 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For convenient review, a little explanation here:
Move preCreateSubscriptionForCompactionIfNeeded after initialize. because in preCreateSubscriptionForCompactionIfNeeded it will retrieve the compaction threshold, it could only get the value at the broker level. Only after initialize, namespace and topic level values will load into the topic. So it's the reason to do this. Besides, I think preCreateSubscriptionForCompactionIfNeeded should stay after initialize is more accurate .

@Technoboy-

Technoboy- commented Mar 12, 2022

Copy link
Copy Markdown
Contributor Author

For convenient review, a little explanation here:
Move preCreateSubscriptionForCompactionIfNeeded after initialize. because in preCreateSubscriptionForCompactionIfNeeded it will retrieve the compaction threshold, it could only get the value at the broker level. Only after initialize, namespace and topic level values will load into the topic. So it's the reason to do this. Besides, I think preCreateSubscriptionForCompactionIfNeeded should stay after initialize is more accurate .

eolivelli
eolivelli approved these changes Mar 12, 2022
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am fine with this PR but the scope is larger than only the heartbeat topic: we are changing the topic creation flow.

Please update the title accordingly or split the patch into two parts

@Technoboy- Technoboy- changed the title Optimize heartbeat topic not trigger compaction. Changing the topic creation flow and optimize heartbeat topic not trigger compaction. Mar 12, 2022
@Technoboy-

Technoboy- commented Mar 12, 2022

Copy link
Copy Markdown
Contributor Author

I am fine with this PR but the scope is larger than only the heartbeat topic: we are changing the topic creation flow.

Please update the title accordingly or split the patch into two parts

Thanks @eolivelli . I have changed the title.

@Technoboy-

Technoboy- commented Mar 14, 2022

Copy link
Copy Markdown
Contributor Author

Could you help review this ?@Jason918 @hangc0276

@codelipenghui codelipenghui merged commit d02bd73 into apache:master Mar 14, 2022
codelipenghui pushed a commit that referenced this pull request Apr 19, 2022
@Technoboy- @codelipenghui
Changing the topic creation flow and optimize heartbeat topic not tri…
a33fd8d 
…gger compaction. (#14643)

### Motivation

When create persistent topic, create compaction subscription(line-1395) is before topic `initialize`(line-1397), it's better to do this after `initialize`:
https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1391-L1398
If we change this part, we can optimize the heartbeat topic not trigger the compaction.
#13611 has made the heartbeat topic as a system topic to avoid deleting by GC. However, system topic is compacted by default. But the heartbeat topic sends msg without a key, so trigger compaction is meaningful.  So it's better to skip the heartbeat topic to do the compaction.

### Modification
- Move `preCreateSubscriptionForCompactionIfNeeded` after `initialize`.
- Add heartbeat topic not trigger compaction.

(cherry picked from commit d02bd73)
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@Technoboy-
Changing the topic creation flow and optimize heartbeat topic not tri…
4dba87c 
…gger compaction. (apache#14643)

### Motivation

When create persistent topic, create compaction subscription(line-1395) is before topic `initialize`(line-1397), it's better to do this after `initialize`:
https://github.com/apache/pulsar/blob/ad2cc2d38280b7dd0f056ee981ec8d3b157e3526/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1391-L1398
If we change this part, we can optimize the heartbeat topic not trigger the compaction.
apache#13611 has made the heartbeat topic as a system topic to avoid deleting by GC. However, system topic is compacted by default. But the heartbeat topic sends msg without a key, so trigger compaction is meaningful.  So it's better to skip the heartbeat topic to do the compaction.

### Modification
- Move `preCreateSubscriptionForCompactionIfNeeded` after `initialize`.
- Add heartbeat topic not trigger compaction.
@Technoboy- Technoboy- mentioned this pull request Apr 21, 2022
Merged
1 task
codelipenghui pushed a commit that referenced this pull request Apr 25, 2022
@Technoboy-
[improve][broker] Tidy up the system topic. (#15252)
aa4df1b 
### Motivation

For #13520, #14643, #14949, we fix some issues related to system topic but result in checking the system topic in different method. So it's better to tidy up the system topic.

So put these system topic names into a new class called SystemTopicNames.
lhotari added a commit to datastax/pulsar that referenced this pull request May 5, 2022
@lhotari
Don't compact healthcheck topic
d17110a 
- backport relevant change from apache#14643 to branch-2.7
@lhotari

lhotari commented May 10, 2022

Copy link
Copy Markdown
Member

#13611 has made the heartbeat topic as a system topic to avoid deleting by GC. However, system topic is compacted by default. But the heartbeat topic sends msg without a key, so trigger compaction is meaningful. So it's better to skip the heartbeat topic to do the compaction.

@Technoboy- Do we need to cherry-pick/backport this PR to the branches which include #13611 / #13816 ?

@lhotari

lhotari commented Jul 11, 2022

Copy link
Copy Markdown
Member

@Technoboy- Do we need to cherry-pick/backport this PR to the branches which include #13611 / #13816 ?

@Technoboy- Technoboy- deleted the fix-healthcheck-compaction branch August 10, 2022 05:52
@TakaHiR07 TakaHiR07 mentioned this pull request Oct 13, 2023
Closed
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jason918 Jason918 Jason918 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@mattisonchao mattisonchao mattisonchao approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@michaeljmarshall michaeljmarshall Awaiting requested review from michaeljmarshall

@315157973 315157973 Awaiting requested review from 315157973

@aloyszhang aloyszhang Awaiting requested review from aloyszhang

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@hezhangjian hezhangjian Awaiting requested review from hezhangjian

@zymap zymap Awaiting requested review from zymap

Assignees

@Technoboy- Technoboy-

Labels

cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.10.1

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

7 participants

@Technoboy- @lhotari @Jason918 @eolivelli @codelipenghui @RobertIndie @mattisonchao