Skip to content

Fix update replication cluster but not update replicator. - #14570

Merged
codelipenghui merged 18 commits into
apache:masterfrom
Technoboy-:fix-replicator-1
Mar 17, 2022
Merged

Fix update replication cluster but not update replicator.#14570
codelipenghui merged 18 commits into
apache:masterfrom
Technoboy-:fix-replicator-1

Conversation

@Technoboy-

@Technoboy- Technoboy- commented Mar 6, 2022

Copy link
Copy Markdown
Contributor

Motivation

When updating replication-cluster for a topic from CLI, the broker doesn't update the replicator.

Documentation

  • no-need-doc

@Technoboy- Technoboy- self-assigned this Mar 6, 2022
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Mar 6, 2022
Jason918
Jason918 approved these changes Mar 7, 2022
View reviewed changes

@Jason918 Jason918 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.

LGTM

@Jason918

Jason918 commented Mar 7, 2022

Copy link
Copy Markdown
Contributor

@AnonHxy Please help review this.

codelipenghui
codelipenghui approved these changes Mar 7, 2022
View reviewed changes
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
@codelipenghui codelipenghui added this to the 2.11.0 milestone Mar 7, 2022
@lhotari
lhotari requested review from merlimat and rdhabalia March 8, 2022 13:48
lhotari
lhotari requested changes Mar 8, 2022
View reviewed changes

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems that this PR would revert #232 changes. @rdhabalia would you be able to review this PR to ensure that it doesn't break things?

@Technoboy-

Technoboy- commented Mar 10, 2022

Copy link
Copy Markdown
Contributor Author

It seems that this PR would revert #232 changes. @rdhabalia would you be able to review this PR to ensure that it doesn't break things?

BTW, this pr relies on #14605

@Technoboy-

Technoboy- commented Mar 11, 2022

Copy link
Copy Markdown
Contributor Author

It seems that this PR would revert #232 changes. @rdhabalia would you be able to review this PR to ensure that it doesn't break things?

I think #232 has added the test to coverage this.

@Technoboy-

Technoboy- commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

Hi, @lhotari , could you help review this again ?

@lhotari

lhotari commented Mar 15, 2022

Copy link
Copy Markdown
Member

Hi, @lhotari , could you help review this again ?

I'd rather have @rdhabalia take a look since he knows this area well.
To me it looks like this PR is removing changes made in #232. It would be good to find an explanation of why that code was there in the first place and why it's now safe to remove.

@Technoboy-

Technoboy- commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

Hi, @lhotari , could you help review this again ?

I'd rather have @rdhabalia take a look since he knows this area well. To me it looks like this PR is removing changes made in #232. It would be good to find an explanation of why that code was there in the first place and why it's now safe to remove.

Yes, we roll back the code. But the test is there.

@Technoboy-

Technoboy- commented Mar 15, 2022

Copy link
Copy Markdown
Contributor Author

Hi @merlimat , could you give some idea about this pr ?

@codelipenghui codelipenghui mentioned this pull request Mar 15, 2022
Closed
@Technoboy-

Technoboy- commented Mar 16, 2022

Copy link
Copy Markdown
Contributor Author

Hi, @lhotari , could you help review this again ?

I'd rather have @rdhabalia take a look since he knows this area well. To me it looks like this PR is removing changes made in #232. It would be good to find an explanation of why that code was there in the first place and why it's now safe to remove.

Hi @lhotari , I remove the modification of startReplProducers. #232 is too old to find the problem at that time. So keep it as the original implementation, I will open another issue to track.
As some users occur the above issue, we need this pr.

lhotari
lhotari approved these changes Mar 17, 2022
View reviewed changes

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@codelipenghui
codelipenghui merged commit bcbeb53 into apache:master Mar 17, 2022
aparajita89 pushed a commit to aparajita89/pulsar that referenced this pull request Mar 21, 2022
@Technoboy- Aparajita Singh
Fix update replication cluster but not update replicator. (apache#14570)
fe18f79
codelipenghui pushed a commit that referenced this pull request Apr 19, 2022
@Technoboy- @codelipenghui
Fix update replication cluster but not update replicator. (#14570)
1a20f96 
(cherry picked from commit bcbeb53)
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@Technoboy- nicklixinyang
Fix update replication cluster but not update replicator. (apache#14570)
01f6d1c
@Technoboy-
Technoboy- deleted the fix-replicator-1 branch August 10, 2022 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lhotari lhotari lhotari approved these changes
@Jason918 Jason918 Jason918 approved these changes
@AnonHxy AnonHxy AnonHxy approved these changes
@codelipenghui codelipenghui codelipenghui approved these changes
@yuruguo yuruguo yuruguo approved these changes
@merlimat merlimat Awaiting requested review from merlimat
@rdhabalia rdhabalia Awaiting requested review from rdhabalia

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.

6 participants

@Technoboy- @Jason918 @lhotari @AnonHxy @codelipenghui @yuruguo