You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "TakaHiR07 (via GitHub)" <gi...@apache.org> on 2023/09/04 09:05:41 UTC
[GitHub] [pulsar] TakaHiR07 opened a new pull request, #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
TakaHiR07 opened a new pull request, #21087:
URL: https://github.com/apache/pulsar/pull/21087
### Motivation
Related to https://github.com/apache/pulsar/pull/20683. When reading the code of master-branch and branch-2.10, I found we can not remove duplicate topics name when deleteNamespace, because the implementation of internalDeletePartitionedTopicsAsync() is different in two branch. In master-branch, if we remove topics name in allUserCreatedTopics and allSystemTopic, we actually not delete the topic. So the first time delete namespace will throw exception exactly.
#### branch-2.10 :
#### master-branch :
### Modifications
revert the modification.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/TakaHiR07/pulsar/pull/14
--
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- closed pull request #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
URL: https://github.com/apache/pulsar/pull/21087
--
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] codecov-commenter commented on pull request #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21087:
URL: https://github.com/apache/pulsar/pull/21087#issuecomment-1704948938
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21087?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#21087](https://app.codecov.io/gh/apache/pulsar/pull/21087?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d96da99) into [master](https://app.codecov.io/gh/apache/pulsar/commit/63d9eaf974d0acf59ea8fa11cc75c08b784cd38d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (63d9eaf) will **increase** coverage by `0.80%`.
> Report is 60 commits behind head on master.
> The diff coverage is `60.00%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/21087?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #21087 +/- ##
============================================
+ Coverage 36.92% 37.72% +0.80%
- Complexity 12187 13008 +821
============================================
Files 1698 1698
Lines 129846 134416 +4570
Branches 14163 15285 +1122
============================================
+ Hits 47947 50714 +2767
- Misses 75570 77141 +1571
- Partials 6329 6561 +232
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21087/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/21087/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.16% <20.00%> (+0.01%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/21087/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `25.05% <20.00%> (-0.10%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21087/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `33.30% <60.00%> (+1.22%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/pulsar/pull/21087?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...pache/pulsar/broker/admin/impl/NamespacesBase.java](https://app.codecov.io/gh/apache/pulsar/pull/21087?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL05hbWVzcGFjZXNCYXNlLmphdmE=) | `31.10% <ø> (-0.20%)` | :arrow_down: |
| [...rg/apache/pulsar/compaction/TwoPhaseCompactor.java](https://app.codecov.io/gh/apache/pulsar/pull/21087?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbXBhY3Rpb24vVHdvUGhhc2VDb21wYWN0b3IuamF2YQ==) | `75.98% <58.33%> (+5.80%)` | :arrow_up: |
| [...g/apache/pulsar/client/impl/RawBatchConverter.java](https://app.codecov.io/gh/apache/pulsar/pull/21087?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1Jhd0JhdGNoQ29udmVydGVyLmphdmE=) | `75.32% <66.66%> (+1.64%)` | :arrow_up: |
... and [126 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21087/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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- merged pull request #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- merged PR #21087:
URL: https://github.com/apache/pulsar/pull/21087
--
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 #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #21087:
URL: https://github.com/apache/pulsar/pull/21087#discussion_r1314451134
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -279,12 +279,6 @@ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTime
return old;
});
}
- allUserCreatedTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- allSystemTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- topicPolicy.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
Review Comment:
Is it possible to have a test to protect the behavior?
--
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 #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #21087:
URL: https://github.com/apache/pulsar/pull/21087#issuecomment-1700997444
yes, right. When releasing 2.10, there was a flaky test, we found it's related to delete duplicated topic. #20685
Then we cherry-pick the logic to the master by #20683. Unfortunately, `internalDeletePartitionedTopicsAsync` has changed in the master branch.
--
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] TakaHiR07 commented on a diff in pull request #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #21087:
URL: https://github.com/apache/pulsar/pull/21087#discussion_r1314547217
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -279,12 +279,6 @@ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTime
return old;
});
}
- allUserCreatedTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- allSystemTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- topicPolicy.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
Review Comment:
https://github.com/apache/pulsar/blob/e59c850753f2a1d08e4df63bf8862c7cb5db4e71/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L328-L342 The retry logic make it hard to add test. Because this wrong behaviour also throw KeeperException.NotEmptyException, which would be caught and retry internalRetryableDeleteNamespaceAsync0(). Actually it should succeed without retry
--
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] TakaHiR07 commented on a diff in pull request #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #21087:
URL: https://github.com/apache/pulsar/pull/21087#discussion_r1314547217
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -279,12 +279,6 @@ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTime
return old;
});
}
- allUserCreatedTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- allSystemTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- topicPolicy.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
Review Comment:
The retry logic in https://github.com/apache/pulsar/blob/e59c850753f2a1d08e4df63bf8862c7cb5db4e71/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L328-L342 make it hard to add test. Because this wrong behaviour also throw KeeperException.NotEmptyException, which would be caught and retry internalRetryableDeleteNamespaceAsync0(). Actually it should succeed without retry
--
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] TakaHiR07 commented on a diff in pull request #21087: [fix][broker] revert remove duplicate topics name when deleteNamespace
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on code in PR #21087:
URL: https://github.com/apache/pulsar/pull/21087#discussion_r1314547217
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -279,12 +279,6 @@ private void internalRetryableDeleteNamespaceAsync0(boolean force, int retryTime
return old;
});
}
- allUserCreatedTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- allSystemTopics.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
- topicPolicy.removeIf(t ->
- allPartitionedTopics.contains(TopicName.get(t).getPartitionedTopicName()));
Review Comment:
The retry logic in pr-19374 make it hard to add test. Because this wrong behaviour also throw KeeperException.NotEmptyException, which would be caught and retry internalRetryableDeleteNamespaceAsync0(). Actually it should succeed without retry
--
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