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/07/15 09:49:34 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #16622: Skip topics with remote replication producers in topic inactivity check
lhotari opened a new pull request, #16622:
URL: https://github.com/apache/pulsar/pull/16622
### Motivation
- currently there's a problem that the replicators get shutdown in the inactivity check
- this is an improved fix for the issue described in #11382
### Modifications
- skip deleting a topic that has connected remote replication producers.
- move existing check to happen before replicators are closed.
### Additional context
An alternative solution is to disable delete-while-inactive for namespaces that are replicated.
```shell
pulsar-admin namespaces set-inactive-topic-policies tenant/namespace --disable-delete-while-inactive
```
--
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 #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#issuecomment-1211634208
@merlimat @codelipenghui Please review
--
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] mattisonchao commented on pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#issuecomment-1186054030
Question:
1. Do you mean we will delete the topic at the second GC check?
2. I'm not sure If it's a breaking change for the user. Maybe we need to notice this change?
3. This will cause the broker to have a large number of unused producers, right?
--
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] mattisonchao commented on a diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957440390
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
I can add the test to cover this behaviour If we need it.
--
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 #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#issuecomment-1229152100
@merlimat @codelipenghui @Technoboy- Ping
--
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 #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#issuecomment-1186106379
> Question:
>
> 1. Do you mean we will delete the topic at the second GC check?
please see how the existing code works in https://github.com/apache/pulsar/blob/c48a3243287c7d775459b6437d9f4b24ed44cf4c/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L2182-L2231 . The existing logic closes replicators that contain no backlog. However if there are connected replication producers, the inactivity deletion will be skipped eventually and it's possible that some replicators were already closed and they won't be restored. This is why this PR is made since it's better to make the check as the first step instead of possibly closing some replicators and then terminating the inactivity deletion.
> 2. I'm not sure If it's a breaking change for the user. Maybe we need to notice this change?
This PR doesn't introduce a breaking change.
> 3. This will cause the broker to have a large number of unused producers, right?
No.
--
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] mattisonchao commented on a diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957401488
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
If we have two clusters, cluster A and cluster B. They enable two-way replication. So, cluster A has remote producer B, and cluster B has remote producer A.
In the previous logic. when the replicator has no more backlog (Cluster A), we will close the replicator of Cluster A. And cluster B will have no remote producer A. In the next round of GC check, if cluster B also has no backlog. At this point, cluster B's replicator will be close and cluster A will also remove cluster B's remote producer. Then in the next new round of GC, we will clean up the topics on both clusters.
But after this PR is modified, if two clusters enable two-way replication, they hold each other's remote producers. At this point, the topic will enter a kind of circular chain, and the check GC will never delete the topic until a replicator is closed.
--
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] mattisonchao commented on a diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957262485
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
From this comment, it seems we have to close the repl producer first. Otherwise, in a two-way replication scenario. We can get to a "deadlock" and the topic is never deleted, even if it has no backlog.
I'm not sure if it's your expected behaviour or if I'm missing some replicator close logic.
--
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] mattisonchao commented on a diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957262485
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
From this comment, it seems we have to close the repl producer first. Otherwise, in a two-way replication scenario. We can get to the "deadlock" and the topic is never deleted.
I'm not sure if it's your expected behaviour or if I'm missing some replicator close logic.
--
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] github-actions[bot] commented on pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#issuecomment-1263024859
The pr had no activity for 30 days, mark with Stale label.
--
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] mattisonchao commented on a diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957262485
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
From this comment, it seems we have to close the repl producer first. Otherwise, in a two-way replication scenario. We can get to a "deadlock" and the topic is never deleted, even if it has no backlog
I'm not sure if it's your expected behaviour or if I'm missing some replicator close logic.
--
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 diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957410170
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
+1
I also have the same concern
And do we have a test for covering the inactive geo topic deletion?
The CI gets passed.
--
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] mattisonchao commented on a diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957261890
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
Review Comment:
From this comment, it seems we have to close the repl producer first. Otherwise, in a two-way replication scenario. We can get to the "deadlock" and the topic is never deleted.
I'm not sure if it's your expected behaviour or if I'm missing some replicator close logic.
--
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] github-actions[bot] commented on pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by github-actions.
github-actions[bot] commented on PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#issuecomment-1411405883
@lhotari Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
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] mattisonchao commented on pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#issuecomment-1186208164
@lhotari
Thanks for your explanation, I'm sorry for my misunderstanding .
--
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 diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957930498
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
> I can add the test to cover this behaviour If we need it.
+1, @mattisonchao
--
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 diff in pull request #16622: Skip topics with remote replication producers in topic inactivity check
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16622:
URL: https://github.com/apache/pulsar/pull/16622#discussion_r957930498
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java:
##########
@@ -2195,6 +2195,15 @@ public void checkGC() {
CompletableFuture<Void> replCloseFuture = new CompletableFuture<>();
if (TopicName.get(topic).isGlobal()) {
+ // topics with remote (replication) producer should be skipped
+ if (hasRemoteProducers()) {
+ if (log.isDebugEnabled()) {
+ log.debug("[{}] Global topic has connected remote producers. Not a candidate for GC",
+ topic);
+ }
+ return;
+ }
+
// For global namespace, close repl producers first.
// Once all repl producers are closed, we can delete the topic,
// provided no remote producers connected to the broker.
Review Comment:
> I can add the test to cover this behaviour If we need it.
+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