You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nodece (via GitHub)" <gi...@apache.org> on 2023/12/21 12:38:53 UTC
[PR] [fix][broker] Fix resourceGroups loading [pulsar]
nodece opened a new pull request, #21781:
URL: https://github.com/apache/pulsar/pull/21781
### Motivation
When `org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListener#loadAllResourceGroups` is unfinished, the `org.apache.pulsar.broker.resourcegroup.ResourceGroupNamespaceConfigListener ` will be created, this will cause the `ResourceGroupNamespaceConfigListener` does not find the `ResourceGroup`:
```
2023-12-21T09:41:18,749+0000 [ForkJoinPool.commonPool-worker-2] ERROR org.apache.pulsar.broker.resourcegroup.ResourceGroupNamespaceConfigListener - Failed to register namespace my-tenant/my-namespace with resource group rg-1
org.apache.pulsar.client.admin.PulsarAdminException: Resource group does not exist: rg-1
at org.apache.pulsar.broker.resourcegroup.ResourceGroupService.checkResourceGroupExists(ResourceGroupService.java:417) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at org.apache.pulsar.broker.resourcegroup.ResourceGroupService.registerNameSpace(ResourceGroupService.java:249) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at org.apache.pulsar.broker.resourcegroup.ResourceGroupNamespaceConfigListener.reconcileNamespaceResourceGroup(ResourceGroupNamespaceConfigListener.java:127) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at org.apache.pulsar.broker.resourcegroup.ResourceGroupNamespaceConfigListener.lambda$updateNamespaceResourceGroup$0(ResourceGroupNamespaceConfigListener.java:69) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:863) ~[?:?]
at java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:841) ~[?:?]
at java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:483) ~[?:?]
at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373) ~[?:?]
at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182) ~[?:?]
at java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655) ~[?:?]
at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622) ~[?:?]
at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165) ~[?:?]
```
### Modifications
- Make `loadAllResourceGroups` return the `CompletableFuture<Void>`, and wait for the return value to be done in the constructor.
- Make `loadAllNamespaceResourceGroups` return the `CompletableFuture<Void>`, and wait for the return value to be done in the constructor.
### Verifying this change
Two tests have been added to verify the fail to load the resource.
### 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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1868487386
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21781?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `10 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`b944f10`)](https://app.codecov.io/gh/apache/pulsar/commit/b944f10d2a9bac2fbe19dc83bf250a276f1ed4d1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.37% compared to head [(`8d49c10`)](https://app.codecov.io/gh/apache/pulsar/pull/21781?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.39%.
> Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/21781?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #21781 +/- ##
============================================
+ Coverage 73.37% 73.39% +0.01%
+ Complexity 32772 32690 -82
============================================
Files 1897 1898 +1
Lines 140647 140674 +27
Branches 15489 15492 +3
============================================
+ Hits 103206 103243 +37
+ Misses 29363 29338 -25
- Partials 8078 8093 +15
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21781/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/21781/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.21% <56.86%> (+0.05%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/21781/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.92% <56.86%> (+0.26%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21781/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.66% <80.39%> (-0.01%)` | :arrow_down: |
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](https://app.codecov.io/gh/apache/pulsar/pull/21781?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ker/resourcegroup/ResourceGroupConfigListener.java](https://app.codecov.io/gh/apache/pulsar/pull/21781?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9yZXNvdXJjZWdyb3VwL1Jlc291cmNlR3JvdXBDb25maWdMaXN0ZW5lci5qYXZh) | `80.26% <81.81%> (+2.79%)` | :arrow_up: |
| [...rcegroup/ResourceGroupNamespaceConfigListener.java](https://app.codecov.io/gh/apache/pulsar/pull/21781?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9yZXNvdXJjZWdyb3VwL1Jlc291cmNlR3JvdXBOYW1lc3BhY2VDb25maWdMaXN0ZW5lci5qYXZh) | `70.58% <79.31%> (+2.73%)` | :arrow_up: |
... and [78 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/21781/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1537679188
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -140,7 +166,10 @@ public void accept(Notification notification) {
Optional<String> rgName = ResourceGroupResources.resourceGroupNameFromPath(notifyPath);
if ((notification.getType() == NotificationType.ChildrenChanged)
|| (notification.getType() == NotificationType.Created)) {
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync().exceptionally((ex) -> {
+ LOG.error("Exception when fetching resource groups", ex);
Review Comment:
I think we should print the notify path in the log so we can know which update fails.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436219698
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
+ PulsarService pulsarService = mock(PulsarService.class);
+ PulsarResources pulsarResources = mock(PulsarResources.class);
+
+ doReturn(pulsarResources).when(pulsarService).getPulsarResources();
+
+ ResourceGroupService resourceGroupService = mock(ResourceGroupService.class);
+ ResourceGroupResources resourceGroupResources = mock(ResourceGroupResources.class);
Review Comment:
This test works fine locally, we can also mock getStore, but getStore shouldn't be called.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436227866
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
> This root cause loadAllResourceGroupsAsync is async call, we need to wait for this loading to complete, and then call new ResourceGroupNamespaceConfigListener.
You added 2 tests, and they only did the things below:
- call `new ResourceGroupNamespaceConfigListener`
- inject an Ex in the constructor of `ResourceGroupNamespaceConfigListener`
They are too simple to let others know why the error `Resource group does not exist: rg-1` throws, and why the current change can avoid the error.
I just know that this patch will make `new ResourceGroupNamespaceConfigListener` get an Ex immediately if `rgResources.listResourceGroupsAsync` throws an Ex, Pulsar ignored the Ex of `rgResources.listResourceGroupsAsync` before.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1517129576
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -49,44 +55,55 @@ public class ResourceGroupConfigListener implements Consumer<Notification> {
private final ResourceGroupResources rgResources;
private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ @SneakyThrows
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync()
+ .get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
Review Comment:
Done.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436212416
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
+ PulsarService pulsarService = mock(PulsarService.class);
+ PulsarResources pulsarResources = mock(PulsarResources.class);
+
+ doReturn(pulsarResources).when(pulsarService).getPulsarResources();
+
+ ResourceGroupService resourceGroupService = mock(ResourceGroupService.class);
+ ResourceGroupResources resourceGroupResources = mock(ResourceGroupResources.class);
Review Comment:
This test is used to verify the resource groups failed to load, the `resourceGroupResources.getStore()` is never called and is unreachable.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1534024939
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -41,52 +53,73 @@
* @see <a href="https://github.com/apache/pulsar/wiki/PIP-82%3A-Tenant-and-namespace-level-rate-limiting">Global-quotas</a>
*
*/
-public class ResourceGroupConfigListener implements Consumer<Notification> {
+public class ResourceGroupConfigListener implements Consumer<Notification>, AutoCloseable {
private static final Logger LOG = LoggerFactory.getLogger(ResourceGroupConfigListener.class);
private final ResourceGroupService rgService;
private final PulsarService pulsarService;
private final ResourceGroupResources rgResources;
- private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private volatile ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private final ScheduledExecutorService executorService;
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
this.rgResources.getStore().registerListener(this);
- rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(
- rgService, pulsarService, this);
+ this.executorService =
+ Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-resource-group-config"));
+ execute(() -> loadAllResourceGroupsWithRetryAsync(0));
}
- private void loadAllResourceGroups() {
- rgResources.listResourceGroupsAsync().whenCompleteAsync((rgList, ex) -> {
- if (ex != null) {
- LOG.error("Exception when fetching resource groups", ex);
- return;
+ @SneakyThrows
+ private void loadAllResourceGroupsWithRetryAsync(long retry) {
+ loadAllResourceGroupsAsync().exceptionally(e -> {
+ long nextRetry = retry + 1;
+ long delay = 500 * nextRetry;
+ LOG.error("Failed to load all resource groups during initialization, retrying after {}ms", delay);
+ schedule(() -> loadAllResourceGroupsWithRetryAsync(nextRetry), delay);
+ throw new RuntimeException(e);
+ }).thenAccept(__ -> {
+ if (rgNamespaceConfigListener == null) {
+ try{
+ rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(rgService, pulsarService, this);
+ }catch (Exception e) {
+ e.printStackTrace();
+ }
}
+ });
+ }
+
+ private CompletableFuture<Void> loadAllResourceGroupsAsync() {
+ return rgResources.listResourceGroupsAsync().thenComposeAsync(rgList -> {
Review Comment:
we can use the executor for this async
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -41,52 +53,73 @@
* @see <a href="https://github.com/apache/pulsar/wiki/PIP-82%3A-Tenant-and-namespace-level-rate-limiting">Global-quotas</a>
*
*/
-public class ResourceGroupConfigListener implements Consumer<Notification> {
+public class ResourceGroupConfigListener implements Consumer<Notification>, AutoCloseable {
private static final Logger LOG = LoggerFactory.getLogger(ResourceGroupConfigListener.class);
private final ResourceGroupService rgService;
private final PulsarService pulsarService;
private final ResourceGroupResources rgResources;
- private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private volatile ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private final ScheduledExecutorService executorService;
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
this.rgResources.getStore().registerListener(this);
- rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(
- rgService, pulsarService, this);
+ this.executorService =
+ Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-resource-group-config"));
+ execute(() -> loadAllResourceGroupsWithRetryAsync(0));
}
- private void loadAllResourceGroups() {
- rgResources.listResourceGroupsAsync().whenCompleteAsync((rgList, ex) -> {
- if (ex != null) {
- LOG.error("Exception when fetching resource groups", ex);
- return;
+ @SneakyThrows
+ private void loadAllResourceGroupsWithRetryAsync(long retry) {
+ loadAllResourceGroupsAsync().exceptionally(e -> {
+ long nextRetry = retry + 1;
+ long delay = 500 * nextRetry;
+ LOG.error("Failed to load all resource groups during initialization, retrying after {}ms", delay);
+ schedule(() -> loadAllResourceGroupsWithRetryAsync(nextRetry), delay);
+ throw new RuntimeException(e);
+ }).thenAccept(__ -> {
+ if (rgNamespaceConfigListener == null) {
+ try{
Review Comment:
checkstyle
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1537256198
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -41,52 +51,68 @@
* @see <a href="https://github.com/apache/pulsar/wiki/PIP-82%3A-Tenant-and-namespace-level-rate-limiting">Global-quotas</a>
*
*/
-public class ResourceGroupConfigListener implements Consumer<Notification> {
+public class ResourceGroupConfigListener implements Consumer<Notification>, AutoCloseable {
private static final Logger LOG = LoggerFactory.getLogger(ResourceGroupConfigListener.class);
private final ResourceGroupService rgService;
private final PulsarService pulsarService;
private final ResourceGroupResources rgResources;
- private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private volatile ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private final ScheduledExecutorService executorService;
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
this.rgResources.getStore().registerListener(this);
- rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(
- rgService, pulsarService, this);
+ this.executorService =
Review Comment:
Good idea.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1537215291
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -140,7 +166,10 @@ public void accept(Notification notification) {
Optional<String> rgName = ResourceGroupResources.resourceGroupNameFromPath(notifyPath);
if ((notification.getType() == NotificationType.ChildrenChanged)
|| (notification.getType() == NotificationType.Created)) {
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync().exceptionally((ex) -> {
+ LOG.error("Exception when fetching resource groups", ex);
Review Comment:
How about adding the "notify path" to the error log?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -41,52 +51,68 @@
* @see <a href="https://github.com/apache/pulsar/wiki/PIP-82%3A-Tenant-and-namespace-level-rate-limiting">Global-quotas</a>
*
*/
-public class ResourceGroupConfigListener implements Consumer<Notification> {
+public class ResourceGroupConfigListener implements Consumer<Notification>, AutoCloseable {
private static final Logger LOG = LoggerFactory.getLogger(ResourceGroupConfigListener.class);
private final ResourceGroupService rgService;
private final PulsarService pulsarService;
private final ResourceGroupResources rgResources;
- private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private volatile ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private final ScheduledExecutorService executorService;
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
this.rgResources.getStore().registerListener(this);
- rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(
- rgService, pulsarService, this);
+ this.executorService =
Review Comment:
How about reuse `pulsar.getExecutor()`?
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436225087
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
But for now implementation, you ignored the Exception when accepting a Notification of resource group changes, it would cause the same error you mentioned in Motivation, 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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1534138906
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -41,52 +53,73 @@
* @see <a href="https://github.com/apache/pulsar/wiki/PIP-82%3A-Tenant-and-namespace-level-rate-limiting">Global-quotas</a>
*
*/
-public class ResourceGroupConfigListener implements Consumer<Notification> {
+public class ResourceGroupConfigListener implements Consumer<Notification>, AutoCloseable {
private static final Logger LOG = LoggerFactory.getLogger(ResourceGroupConfigListener.class);
private final ResourceGroupService rgService;
private final PulsarService pulsarService;
private final ResourceGroupResources rgResources;
- private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private volatile ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private final ScheduledExecutorService executorService;
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
this.rgResources.getStore().registerListener(this);
- rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(
- rgService, pulsarService, this);
+ this.executorService =
+ Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-resource-group-config"));
+ execute(() -> loadAllResourceGroupsWithRetryAsync(0));
}
- private void loadAllResourceGroups() {
- rgResources.listResourceGroupsAsync().whenCompleteAsync((rgList, ex) -> {
- if (ex != null) {
- LOG.error("Exception when fetching resource groups", ex);
- return;
+ @SneakyThrows
+ private void loadAllResourceGroupsWithRetryAsync(long retry) {
+ loadAllResourceGroupsAsync().exceptionally(e -> {
+ long nextRetry = retry + 1;
+ long delay = 500 * nextRetry;
+ LOG.error("Failed to load all resource groups during initialization, retrying after {}ms", delay);
+ schedule(() -> loadAllResourceGroupsWithRetryAsync(nextRetry), delay);
+ throw new RuntimeException(e);
+ }).thenAccept(__ -> {
+ if (rgNamespaceConfigListener == null) {
+ try{
+ rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(rgService, pulsarService, this);
+ }catch (Exception e) {
+ e.printStackTrace();
+ }
}
+ });
+ }
+
+ private CompletableFuture<Void> loadAllResourceGroupsAsync() {
+ return rgResources.listResourceGroupsAsync().thenComposeAsync(rgList -> {
Review Comment:
`executorService` only is used to retry the `loadAllResourceGroupsAsync()`.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1434147352
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -49,44 +55,55 @@ public class ResourceGroupConfigListener implements Consumer<Notification> {
private final ResourceGroupResources rgResources;
private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ @SneakyThrows
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync()
+ .get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
Review Comment:
Better not to call with timeout
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1868314031
/pulsarbot rerun-failure-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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436213007
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -49,44 +55,55 @@ public class ResourceGroupConfigListener implements Consumer<Notification> {
private final ResourceGroupResources rgResources;
private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ @SneakyThrows
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync()
+ .get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
Review Comment:
Could I use `get()` or `join()`?
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436215958
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
I think you should add a test that responds to an error `Resource group does not exist: rg-1`, the same as the case you provided in the motivation.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436213390
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
+ PulsarService pulsarService = mock(PulsarService.class);
+ PulsarResources pulsarResources = mock(PulsarResources.class);
+
+ doReturn(pulsarResources).when(pulsarService).getPulsarResources();
+
+ ResourceGroupService resourceGroupService = mock(ResourceGroupService.class);
+ ResourceGroupResources resourceGroupResources = mock(ResourceGroupResources.class);
Review Comment:
> This test is used to verify the resource groups failed to load, the resourceGroupResources.getStore() is never called and is unreachable.
After run the test, you will get the below error
```
java.lang.AssertionError: Expected ExecutionException to be thrown, but NullPointerException was thrown
at org.testng.Assert.expectThrows(Assert.java:2450)
at org.testng.Assert.expectThrows(Assert.java:2421)
at org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListenerTest.testLoadAllResourceGroupsAsyncFailed(ResourceGroupConfigListenerTest.java:315)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:677)
at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:221)
at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:969)
at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:194)
at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:148)
at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at org.testng.TestRunner.privateRun(TestRunner.java:829)
at org.testng.TestRunner.run(TestRunner.java:602)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:437)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:431)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:391)
at org.testng.SuiteRunner.run(SuiteRunner.java:330)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1256)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1176)
at org.testng.TestNG.runSuites(TestNG.java:1099)
at org.testng.TestNG.run(TestNG.java:1067)
at com.intellij.rt.testng.IDEARemoteTestNG.run(IDEARemoteTestNG.java:66)
at com.intellij.rt.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:105)
Caused by: java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.metadata.api.MetadataStore.registerListener(java.util.function.Consumer)" because the return value of "org.apache.pulsar.broker.resources.ResourceGroupResources.getStore()" is null
at org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListener.<init>(ResourceGroupConfigListener.java:57)
at org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListenerTest.lambda$testLoadAllResourceGroupsAsyncFailed$11(ResourceGroupConfigListenerTest.java:316)
at org.testng.Assert.expectThrows(Assert.java:2440)
... 29 more
```
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436215958
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
I think you should add a test that responds to an error `Resource group does not exist: rg-1`, the same as the case you provided in the motivation.
```
2023-12-21T09:41:18,749+0000 [ForkJoinPool.commonPool-worker-2] ERROR org.apache.pulsar.broker.resourcegroup.ResourceGroupNamespaceConfigListener - Failed to register namespace my-tenant/my-namespace with resource group rg-1
org.apache.pulsar.client.admin.PulsarAdminException: Resource group does not exist: rg-1
at org.apache.pulsar.broker.resourcegroup.ResourceGroupService.checkResourceGroupExists(ResourceGroupService.java:417) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at org.apache.pulsar.broker.resourcegroup.ResourceGroupService.registerNameSpace(ResourceGroupService.java:249) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at org.apache.pulsar.broker.resourcegroup.ResourceGroupNamespaceConfigListener.reconcileNamespaceResourceGroup(ResourceGroupNamespaceConfigListener.java:127) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at org.apache.pulsar.broker.resourcegroup.ResourceGroupNamespaceConfigListener.lambda$updateNamespaceResourceGroup$0(ResourceGroupNamespaceConfigListener.java:69) ~[org.apache.pulsar-pulsar-broker-3.1.1.jar:3.1.1]
at java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:863) ~[?:?]
at java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:841) ~[?:?]
at java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:483) ~[?:?]
at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:373) ~[?:?]
at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1182) ~[?:?]
at java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1655) ~[?:?]
at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1622) ~[?:?]
at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:165) ~[?:?]
```
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436227866
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
> This root cause loadAllResourceGroupsAsync is async call, we need to wait for this loading to complete, and then call new ResourceGroupNamespaceConfigListener.
You added 2 tests, and they only did the things below:
- call `new ResourceGroupNamespaceConfigListener`
- inject an Ex in the constructor of `ResourceGroupNamespaceConfigListener`
They are too simple to let others know why the error `Resource group does not exist: rg-1` throws, and why the current change can avoid the error.
Regarding your tests, I just know that this patch will make `new ResourceGroupNamespaceConfigListener` get an Ex immediately if `rgResources.listResourceGroupsAsync` throws an Ex, Pulsar ignored the Ex of `rgResources.listResourceGroupsAsync` before.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1534195770
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -41,52 +53,73 @@
* @see <a href="https://github.com/apache/pulsar/wiki/PIP-82%3A-Tenant-and-namespace-level-rate-limiting">Global-quotas</a>
*
*/
-public class ResourceGroupConfigListener implements Consumer<Notification> {
+public class ResourceGroupConfigListener implements Consumer<Notification>, AutoCloseable {
private static final Logger LOG = LoggerFactory.getLogger(ResourceGroupConfigListener.class);
private final ResourceGroupService rgService;
private final PulsarService pulsarService;
private final ResourceGroupResources rgResources;
- private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private volatile ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ private final ScheduledExecutorService executorService;
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
this.rgResources.getStore().registerListener(this);
- rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(
- rgService, pulsarService, this);
+ this.executorService =
+ Executors.newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-resource-group-config"));
+ execute(() -> loadAllResourceGroupsWithRetryAsync(0));
}
- private void loadAllResourceGroups() {
- rgResources.listResourceGroupsAsync().whenCompleteAsync((rgList, ex) -> {
- if (ex != null) {
- LOG.error("Exception when fetching resource groups", ex);
- return;
+ @SneakyThrows
+ private void loadAllResourceGroupsWithRetryAsync(long retry) {
+ loadAllResourceGroupsAsync().exceptionally(e -> {
+ long nextRetry = retry + 1;
+ long delay = 500 * nextRetry;
+ LOG.error("Failed to load all resource groups during initialization, retrying after {}ms", delay);
+ schedule(() -> loadAllResourceGroupsWithRetryAsync(nextRetry), delay);
+ throw new RuntimeException(e);
+ }).thenAccept(__ -> {
+ if (rgNamespaceConfigListener == null) {
+ try{
Review Comment:
try...catch is unnecessary.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1891667840
https://github.com/apache/pulsar/pull/15126 has been closed, so this PR needs to be reviewed 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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1984998970
The comment has been resolved and can be reviewed 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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1538660637
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -140,7 +166,10 @@ public void accept(Notification notification) {
Optional<String> rgName = ResourceGroupResources.resourceGroupNameFromPath(notifyPath);
if ((notification.getType() == NotificationType.ChildrenChanged)
|| (notification.getType() == NotificationType.Created)) {
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync().exceptionally((ex) -> {
+ LOG.error("Exception when fetching resource groups", ex);
Review Comment:
Ping @poorbarcode
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1537303065
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -140,7 +166,10 @@ public void accept(Notification notification) {
Optional<String> rgName = ResourceGroupResources.resourceGroupNameFromPath(notifyPath);
if ((notification.getType() == NotificationType.ChildrenChanged)
|| (notification.getType() == NotificationType.Created)) {
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync().exceptionally((ex) -> {
+ LOG.error("Exception when fetching resource groups", ex);
Review Comment:
Right, should we add a retry 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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1868482383
/pulsarbot rerun-failure-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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436227866
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
> This root cause loadAllResourceGroupsAsync is async call, we need to wait for this loading to complete, and then call new ResourceGroupNamespaceConfigListener.
You added 2 tests, and they only did the things below:
- call `new ResourceGroupNamespaceConfigListener`
- inject an Ex in the constructor of `ResourceGroupNamespaceConfigListener`
They are too simple to let others know why the error `Resource group does not exist: rg-1` throws, and why the current change can avoid the error.
Right now, I just know that this patch will make `new ResourceGroupNamespaceConfigListener` get an Ex immediately if `rgResources.listResourceGroupsAsync` throws an Ex, Pulsar ignored the Ex of `rgResources.listResourceGroupsAsync` before.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1537253453
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -140,7 +166,10 @@ public void accept(Notification notification) {
Optional<String> rgName = ResourceGroupResources.resourceGroupNameFromPath(notifyPath);
if ((notification.getType() == NotificationType.ChildrenChanged)
|| (notification.getType() == NotificationType.Created)) {
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync().exceptionally((ex) -> {
+ LOG.error("Exception when fetching resource groups", ex);
Review Comment:
This is handled the same way as before.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-2018051169
/pulsarbot rerun-failure-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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1867825305
/pulsarbot rerun-failure-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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1866185260
/pulsarbot rerun-failure-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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-2014266913
/pulsarbot rerun-failure-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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1537260613
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -140,7 +166,10 @@ public void accept(Notification notification) {
Optional<String> rgName = ResourceGroupResources.resourceGroupNameFromPath(notifyPath);
if ((notification.getType() == NotificationType.ChildrenChanged)
|| (notification.getType() == NotificationType.Created)) {
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync().exceptionally((ex) -> {
+ LOG.error("Exception when fetching resource groups", ex);
Review Comment:
Once an update fails, we can not know which one it is, 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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1434775179
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -49,44 +55,55 @@ public class ResourceGroupConfigListener implements Consumer<Notification> {
private final ResourceGroupResources rgResources;
private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ @SneakyThrows
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync()
+ .get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
Review Comment:
Register the listener when the `loadAllResourceGroups` is complete
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436208627
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
+ PulsarService pulsarService = mock(PulsarService.class);
+ PulsarResources pulsarResources = mock(PulsarResources.class);
+
+ doReturn(pulsarResources).when(pulsarService).getPulsarResources();
+
+ ResourceGroupService resourceGroupService = mock(ResourceGroupService.class);
+ ResourceGroupResources resourceGroupResources = mock(ResourceGroupResources.class);
Review Comment:
Because you did not mock the object `resourceGroupResources` completely, the subsequent execution of `resourceGroupResources.getStore()` will throw a null pointer exception, which makes the current test cannot prove the problem.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -49,44 +55,55 @@ public class ResourceGroupConfigListener implements Consumer<Notification> {
private final ResourceGroupResources rgResources;
private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ @SneakyThrows
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync()
+ .get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
Review Comment:
> Better not to call with timeout
+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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#issuecomment-1869576632
Add related issue https://github.com/apache/pulsar/issues/15113
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1537844198
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -140,7 +166,10 @@ public void accept(Notification notification) {
Optional<String> rgName = ResourceGroupResources.resourceGroupNameFromPath(notifyPath);
if ((notification.getType() == NotificationType.ChildrenChanged)
|| (notification.getType() == NotificationType.Created)) {
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync().exceptionally((ex) -> {
+ LOG.error("Exception when fetching resource groups", ex);
Review Comment:
This path is the root path of the resource group, and we also print the notify path in the `org.apache.pulsar.broker.resourcegroup.ResourceGroupConfigListener#accept`.
When an error is thrown, we can find the context.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece merged PR #21781:
URL: https://github.com/apache/pulsar/pull/21781
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1434637713
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -49,44 +55,55 @@ public class ResourceGroupConfigListener implements Consumer<Notification> {
private final ResourceGroupResources rgResources;
private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ @SneakyThrows
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync()
+ .get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
Review Comment:
We must wait for all resource groups to the cache from the metadatastore, do you have any good suggestions?
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436220791
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
This root cause `loadAllResourceGroupsAsync` is async call, we need to wait for this loading to complete, and then call `new ResourceGroupNamespaceConfigListener`.
```java
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
loadAllResourceGroupsAsync()
.get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
this.rgResources.getStore().registerListener(this);
rgNamespaceConfigListener = new ResourceGroupNamespaceConfigListener(
rgService, pulsarService, 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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436214595
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListener.java:
##########
@@ -49,44 +55,55 @@ public class ResourceGroupConfigListener implements Consumer<Notification> {
private final ResourceGroupResources rgResources;
private final ResourceGroupNamespaceConfigListener rgNamespaceConfigListener;
+ @SneakyThrows
public ResourceGroupConfigListener(ResourceGroupService rgService, PulsarService pulsarService) {
this.rgService = rgService;
this.pulsarService = pulsarService;
this.rgResources = pulsarService.getPulsarResources().getResourcegroupResources();
- loadAllResourceGroups();
+ loadAllResourceGroupsAsync()
+ .get(pulsarService.getConfiguration().getMetadataStoreOperationTimeoutSeconds(), TimeUnit.SECONDS);
Review Comment:
> Could I use get() or join()?
I think it is worse than `get timeout`
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436234461
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
> They are too simple to let others know why the error `Resource group does not exist: rg-1` throws, and why the current change can avoid the error.
The PR description has been updated.
Regarding your tests, I just know that this patch will make `new ResourceGroupNamespaceConfigListener` get an Ex immediately if `rgResources.listResourceGroupsAsync` throws an Ex, Pulsar ignored the Ex of `rgResources.listResourceGroupsAsync` before.
Sure, we have to throw Exception in `ResourceGroupConfigListener` and `ResourceGroupNamespaceConfigListener`. If not, the rate limiter is not working.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
> They are too simple to let others know why the error `Resource group does not exist: rg-1` throws, and why the current change can avoid the error.
The PR description has been updated.
> Regarding your tests, I just know that this patch will make `new ResourceGroupNamespaceConfigListener` get an Ex immediately if `rgResources.listResourceGroupsAsync` throws an Ex, Pulsar ignored the Ex of `rgResources.listResourceGroupsAsync` before.
Sure, we have to throw Exception in `ResourceGroupConfigListener` and `ResourceGroupNamespaceConfigListener`. If not, the rate limiter is not working.
--
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
Re: [PR] [fix][broker] Fix ResourceGroups loading [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #21781:
URL: https://github.com/apache/pulsar/pull/21781#discussion_r1436232503
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/resourcegroup/ResourceGroupConfigListenerTest.java:
##########
@@ -288,4 +298,27 @@ private void prepareData() throws PulsarAdminException {
testAddRg.setDispatchRateInBytes(200L);
}
+
+ @Test
+ public void testLoadAllResourceGroupsAsyncFailed() {
Review Comment:
> But for now implementation, you ignored the Exception when accepting a Notification of resource group changes, it would cause the same error you mentioned in Motivation, right?
For the resource group changes(reload/update), it always ignored the Exception.
When new `ResourceGroupConfigListener` and `ResourceGroupNamespaceConfigListener`, they will load the resource groups and namespace to cache to bind the namespace and resource groups. But loading the resource groups is async, so the `ResourceGroupNamespaceConfigListener` cannot find the resource groups from the cache.
--
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