You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "Demogorgon314 (via GitHub)" <gi...@apache.org> on 2023/06/07 12:00:42 UTC
[GitHub] [pulsar] Demogorgon314 opened a new pull request, #20528: [fix][broker] Fix redirect loop when using ExtensibleLoadManager and list in bundle admin API
Demogorgon314 opened a new pull request, #20528:
URL: https://github.com/apache/pulsar/pull/20528
PIP: https://github.com/apache/pulsar/issues/16691
### Motivation
When using `ExtensibleLoadManager` and list in bundle admin API,
it will redirect forever because `isServiceUnitOwned` method is checking the `ownershipCache` as the ownership storage, however, when using `ExtensibleLoadManager`, it stored the ownership to table view.
### Modifications
* Call `isServiceUnitOwnedAsync ` when using `isServiceUnitOwned `.
* Add unit test to cover this case.
### 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 -->
--
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] Demogorgon314 commented on a diff in pull request #20528: [fix][broker] Fix redirect loop when using ExtensibleLoadManager and list in bundle admin API
Posted by "Demogorgon314 (via GitHub)" <gi...@apache.org>.
Demogorgon314 commented on code in PR #20528:
URL: https://github.com/apache/pulsar/pull/20528#discussion_r1222293463
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1108,19 +1108,7 @@ public Set<NamespaceBundle> getOwnedServiceUnits() {
}
public boolean isServiceUnitOwned(ServiceUnitId suName) throws Exception {
- if (suName instanceof TopicName) {
- return isTopicOwnedAsync((TopicName) suName).get();
Review Comment:
Yes, you can see the `isServiceUnitOwnedAsync` method has the same checks.
--
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] Demogorgon314 merged pull request #20528: [fix][broker] Fix redirect loop when using ExtensibleLoadManager and list in bundle admin API
Posted by "Demogorgon314 (via GitHub)" <gi...@apache.org>.
Demogorgon314 merged PR #20528:
URL: https://github.com/apache/pulsar/pull/20528
--
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] Demogorgon314 commented on a diff in pull request #20528: [fix][broker] Fix redirect loop when using ExtensibleLoadManager and list in bundle admin API
Posted by "Demogorgon314 (via GitHub)" <gi...@apache.org>.
Demogorgon314 commented on code in PR #20528:
URL: https://github.com/apache/pulsar/pull/20528#discussion_r1221510899
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1108,19 +1108,7 @@ public Set<NamespaceBundle> getOwnedServiceUnits() {
}
public boolean isServiceUnitOwned(ServiceUnitId suName) throws Exception {
Review Comment:
Yes, let's address it in another 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] heesung-sn commented on a diff in pull request #20528: [fix][broker] Fix redirect loop when using ExtensibleLoadManager and list in bundle admin API
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on code in PR #20528:
URL: https://github.com/apache/pulsar/pull/20528#discussion_r1221897582
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1108,19 +1108,7 @@ public Set<NamespaceBundle> getOwnedServiceUnits() {
}
public boolean isServiceUnitOwned(ServiceUnitId suName) throws Exception {
- if (suName instanceof TopicName) {
- return isTopicOwnedAsync((TopicName) suName).get();
Review Comment:
Are we safe to skip these checks?
--
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 #20528: [fix][broker] Fix redirect loop when using ExtensibleLoadManager and list in bundle admin API
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20528:
URL: https://github.com/apache/pulsar/pull/20528#issuecomment-1581934280
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#20528](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8035be2) into [master](https://app.codecov.io/gh/apache/pulsar/commit/d7f355881b2b1eebf2be6ea262c202660d684fb7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d7f3558) will **increase** coverage by `36.09%`.
> The diff coverage is `43.75%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #20528 +/- ##
=============================================
+ Coverage 36.82% 72.92% +36.09%
- Complexity 12040 31905 +19865
=============================================
Files 1690 1867 +177
Lines 128968 138548 +9580
Branches 14036 15216 +1180
=============================================
+ Hits 47493 101034 +53541
+ Misses 75220 29490 -45730
- Partials 6255 8024 +1769
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.15% <6.25%> (-0.01%)` | :arrow_down: |
| systests | `25.06% <6.25%> (+0.01%)` | :arrow_up: |
| unittests | `72.19% <43.75%> (+40.29%)` | :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.
| [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../pulsar/functions/runtime/JavaInstanceStarter.java](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvZnVuY3Rpb25zL3J1bnRpbWUvSmF2YUluc3RhbmNlU3RhcnRlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...he/pulsar/broker/admin/v2/NonPersistentTopics.java](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9Ob25QZXJzaXN0ZW50VG9waWNzLmphdmE=) | `61.92% <33.33%> (+34.14%)` | :arrow_up: |
| [...pache/pulsar/broker/admin/v2/PersistentTopics.java](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9QZXJzaXN0ZW50VG9waWNzLmphdmE=) | `80.83% <33.33%> (+67.96%)` | :arrow_up: |
| [...unctions/runtime/kubernetes/KubernetesRuntime.java](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWZ1bmN0aW9ucy9ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvZnVuY3Rpb25zL3J1bnRpbWUva3ViZXJuZXRlcy9LdWJlcm5ldGVzUnVudGltZS5qYXZh) | `38.73% <50.00%> (+38.73%)` | :arrow_up: |
| [...ache/pulsar/broker/namespace/NamespaceService.java](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9uYW1lc3BhY2UvTmFtZXNwYWNlU2VydmljZS5qYXZh) | `70.35% <100.00%> (+26.76%)` | :arrow_up: |
| [...n/java/org/apache/pulsar/admin/cli/CmdSources.java](https://app.codecov.io/gh/apache/pulsar/pull/20528?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC10b29scy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2FkbWluL2NsaS9DbWRTb3VyY2VzLmphdmE=) | `45.03% <100.00%> (+45.03%)` | :arrow_up: |
... and [1429 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20528/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] eolivelli commented on a diff in pull request #20528: [fix][broker] Fix redirect loop when using ExtensibleLoadManager and list in bundle admin API
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #20528:
URL: https://github.com/apache/pulsar/pull/20528#discussion_r1221494384
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1108,19 +1108,7 @@ public Set<NamespaceBundle> getOwnedServiceUnits() {
}
public boolean isServiceUnitOwned(ServiceUnitId suName) throws Exception {
Review Comment:
as a follow up work we should remove this synchronous method
--
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