You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/03/06 13:25:25 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #14570: Fix update replication cluster but not update replicator.
Technoboy- opened a new pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570
### Motivation
When updating replication-cluster for a topic from CLI, the broker doesn't update the replicator.
### Documentation
- [x] `no-need-doc`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1067753794
Hi @merlimat , could you give some idea about this pr ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1067753419
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#discussion_r821252352
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -611,28 +610,13 @@ private boolean hasRemoteProducers() {
public CompletableFuture<Void> startReplProducers() {
Review comment:
There are some methods like `checkReplication`,`addProducer` that invoke this method and use `thanAccept`.
So keep the original signature is much more better.
@AnonHxy @codelipenghui
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1064799293
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on a change in pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on a change in pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#discussion_r820804592
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -611,28 +610,13 @@ private boolean hasRemoteProducers() {
public CompletableFuture<Void> startReplProducers() {
Review comment:
This method can return void
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#discussion_r820816415
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -611,28 +610,13 @@ private boolean hasRemoteProducers() {
public CompletableFuture<Void> startReplProducers() {
Review comment:
Yes, just keep the original signature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1068959188
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1067683812
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1063708803
> 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 https://github.com/apache/pulsar/pull/14605
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#discussion_r820415271
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -3063,6 +3046,27 @@ public void onUpdate(TopicPolicies policies) {
});
}
+ private CompletableFuture<Void> checkReplicator() {
Review comment:
There is already a `checkReplication` in this class. It seems contains most of the logic here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1060515738
@AnonHxy Please help review this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#issuecomment-1067624799
Hi, @lhotari , could you help review this again ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#discussion_r820439114
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -3063,6 +3046,27 @@ public void onUpdate(TopicPolicies policies) {
});
}
+ private CompletableFuture<Void> checkReplicator() {
Review comment:
Yes, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570#discussion_r820819520
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -611,28 +610,13 @@ private boolean hasRemoteProducers() {
public CompletableFuture<Void> startReplProducers() {
Review comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #14570: Fix update replication cluster but not update replicator.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14570:
URL: https://github.com/apache/pulsar/pull/14570
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org