You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/22 08:44:21 UTC
[GitHub] [pulsar] lordcheng10 opened a new pull request, #17797: [fix][broker] Update new bundle-range to policies after bundle split
lordcheng10 opened a new pull request, #17797:
URL: https://github.com/apache/pulsar/pull/17797
### Motivation
After the bundle is split, update the new bundle information to admin/policies.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272224372
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r979219028
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -959,6 +970,37 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
});
}
+ /**
+ * Update new bundle-range to admin/policies/namespace.
+ * Update may fail because of concurrent write to Zookeeper.
+ *
+ * @param nsname
+ * @param nsBundles
+ * @throws Exception
+ */
+ private CompletableFuture<Void> updateNamespaceBundlesForPolicies(NamespaceName nsname,
+ NamespaceBundles nsBundles) {
+ checkNotNull(nsname);
+ checkNotNull(nsBundles);
+
+ return pulsar.getPulsarResources().getNamespaceResources().getPoliciesAsync(nsname).thenCompose(optPolicies -> {
Review Comment:
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r985074451
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -884,15 +884,28 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
.thenRun(() -> {
bundleFactory.invalidateBundleCache(bundle.getNamespaceObject());
updateFuture.complete(splittedBundles.getRight());
Review Comment:
It seems `updateFuture` should complete after `updateNamespaceBundlesForPolicies` completes.
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r985103316
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -884,15 +884,28 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
.thenRun(() -> {
bundleFactory.invalidateBundleCache(bundle.getNamespaceObject());
updateFuture.complete(splittedBundles.getRight());
Review Comment:
Fixed,PTAL,thanks! @Jason918
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r990745899
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -959,6 +970,34 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
});
}
+ /**
+ * Update new bundle-range to admin/policies/namespace.
+ * Update may fail because of concurrent write to Zookeeper.
+ *
+ * @param nsname
+ * @param nsBundles
+ * @throws Exception
+ */
+ private CompletableFuture<Void> updateNamespaceBundlesForPolicies(NamespaceName nsname,
+ NamespaceBundles nsBundles) {
+ Objects.requireNonNull(nsname);
+ Objects.requireNonNull(nsBundles);
+
+ return pulsar.getPulsarResources().getNamespaceResources().getPoliciesAsync(nsname).thenCompose(policies -> {
+ if (policies.isPresent()) {
+ return pulsar.getPulsarResources().getNamespaceResources().setPoliciesAsync(nsname, oldPolicies -> {
+ oldPolicies.bundles = nsBundles.getBundlesData();
+ return oldPolicies;
+ });
+ } else {
+ Policies newPolicies = new Policies();
Review Comment:
I wonder if we will remove the existed Policies in a race condition.
--
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] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272313341
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r985026005
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +880,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
+ .exceptionally(e -> {
+ String msg = format("failed to update namespace policies [%s], "
+ + "NamespaceBundle: %s due to %s",
+ nsname.toString(), bundle.getBundleRange(), e.getMessage());
+ LOG.warn(msg);
+ updateFuture.completeExceptionally(
Review Comment:
> I wonder what will happen if updateFuture completeExceptionally here and complete success in L896(after updateNamespaceBundles success).
Local policies were updated successfully, but policies update failed.
> There is no executing order guarantee for these two ops.
I can add an order guarantee: updateNamespaceBundlesForPolicies is only executed after updateNamespaceBundles is executed successfully.
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r984174958
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
The maximum number of retry is 7 ：
<img width="885" alt="image" src="https://user-images.githubusercontent.com/19296967/193182877-79600acf-7776-4264-95dc-593ad6a4e212.png">
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r983318634
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
After the split, several zookeeper paths are updated or created:
1. Each bundle corresponds to the path of the owner broker;
2. Update the local policies path;
Do you want to update the operations of these paths into atomic operations?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r990740842
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
Objects.requireNonNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
- updateNamespaceBundles(nsname, splittedBundles.getLeft())
- .thenRun(() -> {
- bundleFactory.invalidateBundleCache(bundle.getNamespaceObject());
- updateFuture.complete(splittedBundles.getRight());
- }).exceptionally(ex1 -> {
- String msg = format("failed to update namespace policies [%s], "
+ updateNamespaceBundles(nsname, splittedBundles.getLeft()).thenRun(() -> {
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
+ .thenRun(() -> {
+ bundleFactory.invalidateBundleCache(bundle.getNamespaceObject());
+ updateFuture.complete(splittedBundles.getRight());
+ }).exceptionally(e -> {
+ String msg = format("failed to update namespace policies [%s], "
+ + "NamespaceBundle: %s due to %s",
+ nsname.toString(), bundle.getBundleRange(), e.getMessage());
+ LOG.warn(msg);
+ updateFuture.completeExceptionally(
+ new ServiceUnitNotReadyException(msg, e.getCause()));
+ return null;
+ });
Review Comment:
Is it possible to remove line889 to line897？It seems same with line898 to line905
--
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] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272197007
@Technoboy- PTAL,thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r984174096
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
It will retry!
<img width="1265" alt="image" src="https://user-images.githubusercontent.com/19296967/193182665-d78af75c-a22f-4e6b-bfc4-22d4ca57c3af.png">
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r984174096
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
It will retry!
<img width="1265" alt="image" src="https://user-images.githubusercontent.com/19296967/193182665-d78af75c-a22f-4e6b-bfc4-22d4ca57c3af.png">
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
The maximum number of retry is 7 ：
<img width="885" alt="image" src="https://user-images.githubusercontent.com/19296967/193182877-79600acf-7776-4264-95dc-593ad6a4e212.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272513841
LGTM now
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r990754024
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -959,6 +970,34 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
});
}
+ /**
+ * Update new bundle-range to admin/policies/namespace.
+ * Update may fail because of concurrent write to Zookeeper.
+ *
+ * @param nsname
+ * @param nsBundles
+ * @throws Exception
+ */
+ private CompletableFuture<Void> updateNamespaceBundlesForPolicies(NamespaceName nsname,
+ NamespaceBundles nsBundles) {
+ Objects.requireNonNull(nsname);
+ Objects.requireNonNull(nsBundles);
+
+ return pulsar.getPulsarResources().getNamespaceResources().getPoliciesAsync(nsname).thenCompose(policies -> {
+ if (policies.isPresent()) {
+ return pulsar.getPulsarResources().getNamespaceResources().setPoliciesAsync(nsname, oldPolicies -> {
+ oldPolicies.bundles = nsBundles.getBundlesData();
+ return oldPolicies;
+ });
+ } else {
+ Policies newPolicies = new Policies();
Review Comment:
Fixed,PTAL,thanks! @AnonHxy
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
Objects.requireNonNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
- updateNamespaceBundles(nsname, splittedBundles.getLeft())
- .thenRun(() -> {
- bundleFactory.invalidateBundleCache(bundle.getNamespaceObject());
- updateFuture.complete(splittedBundles.getRight());
- }).exceptionally(ex1 -> {
- String msg = format("failed to update namespace policies [%s], "
+ updateNamespaceBundles(nsname, splittedBundles.getLeft()).thenRun(() -> {
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
+ .thenRun(() -> {
+ bundleFactory.invalidateBundleCache(bundle.getNamespaceObject());
+ updateFuture.complete(splittedBundles.getRight());
+ }).exceptionally(e -> {
+ String msg = format("failed to update namespace policies [%s], "
+ + "NamespaceBundle: %s due to %s",
+ nsname.toString(), bundle.getBundleRange(), e.getMessage());
+ LOG.warn(msg);
+ updateFuture.completeExceptionally(
+ new ServiceUnitNotReadyException(msg, e.getCause()));
+ return null;
+ });
Review Comment:
Fixed,PTAL,thanks! @AnonHxy
--
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] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272199845
@aloyszhang PTAL,thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272301972
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272425448
@AnonHxy PTAL,thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1263151606
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272264210
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272197061
@codelipenghui @Technoboy- PTAL,thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272527204
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r985026269
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +880,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
+ .exceptionally(e -> {
+ String msg = format("failed to update namespace policies [%s], "
+ + "NamespaceBundle: %s due to %s",
+ nsname.toString(), bundle.getBundleRange(), e.getMessage());
+ LOG.warn(msg);
+ updateFuture.completeExceptionally(
Review Comment:
I add an order guarantee: updateNamespaceBundlesForPolicies is only executed after updateNamespaceBundles is executed successfully!
@Jason918 PTAL,thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r978231855
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -959,6 +970,37 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
});
}
+ /**
+ * Update new bundle-range to admin/policies/namespace.
+ * Update may fail because of concurrent write to Zookeeper.
+ *
+ * @param nsname
+ * @param nsBundles
+ * @throws Exception
+ */
+ private CompletableFuture<Void> updateNamespaceBundlesForPolicies(NamespaceName nsname,
+ NamespaceBundles nsBundles) {
+ checkNotNull(nsname);
+ checkNotNull(nsBundles);
+
+ return pulsar.getPulsarResources().getNamespaceResources().getPoliciesAsync(nsname).thenCompose(optPolicies -> {
Review Comment:
thenCompose -> thenApply ?
--
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] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272215120
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272238098
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r990748495
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -959,6 +970,34 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
});
}
+ /**
+ * Update new bundle-range to admin/policies/namespace.
+ * Update may fail because of concurrent write to Zookeeper.
+ *
+ * @param nsname
+ * @param nsBundles
+ * @throws Exception
+ */
+ private CompletableFuture<Void> updateNamespaceBundlesForPolicies(NamespaceName nsname,
+ NamespaceBundles nsBundles) {
+ Objects.requireNonNull(nsname);
+ Objects.requireNonNull(nsBundles);
+
+ return pulsar.getPulsarResources().getNamespaceResources().getPoliciesAsync(nsname).thenCompose(policies -> {
+ if (policies.isPresent()) {
+ return pulsar.getPulsarResources().getNamespaceResources().setPoliciesAsync(nsname, oldPolicies -> {
+ oldPolicies.bundles = nsBundles.getBundlesData();
+ return oldPolicies;
+ });
+ } else {
+ Policies newPolicies = new Policies();
Review Comment:
1. Policies are created when the namespace is created.
2. Under normal circumstances, after the bundle is split, the case where the policies do not exist will not appear.
<img width="1007" alt="image" src="https://user-images.githubusercontent.com/19296967/194743893-7315b131-3bf0-44e1-a11c-d1cef8dfe252.png">
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r984186317
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -959,6 +970,37 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
});
}
+ /**
+ * Update new bundle-range to admin/policies/namespace.
+ * Update may fail because of concurrent write to Zookeeper.
+ *
+ * @param nsname
+ * @param nsBundles
+ * @throws Exception
+ */
+ private CompletableFuture<Void> updateNamespaceBundlesForPolicies(NamespaceName nsname,
+ NamespaceBundles nsBundles) {
+ checkNotNull(nsname);
+ checkNotNull(nsBundles);
+
+ return pulsar.getPulsarResources().getNamespaceResources().getPoliciesAsync(nsname).thenCompose(optPolicies -> {
Review Comment:
fixed! PTAL,thanks! @Jason918
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r979219028
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -959,6 +970,37 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
});
}
+ /**
+ * Update new bundle-range to admin/policies/namespace.
+ * Update may fail because of concurrent write to Zookeeper.
+ *
+ * @param nsname
+ * @param nsBundles
+ * @throws Exception
+ */
+ private CompletableFuture<Void> updateNamespaceBundlesForPolicies(NamespaceName nsname,
+ NamespaceBundles nsBundles) {
+ checkNotNull(nsname);
+ checkNotNull(nsBundles);
+
+ return pulsar.getPulsarResources().getNamespaceResources().getPoliciesAsync(nsname).thenCompose(optPolicies -> {
Review Comment:
OK
--
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] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272197251
/pulsarbot run-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
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272425240
@codelipenghui @Technoboy- PTAL,thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1269248790
CI passed: https://github.com/lordcheng10/pulsar/pull/17
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r978212251
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
What about update `updateNamespaceBundlesForPolicies` success but `updateNamespaceBundles` fail?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r984344809
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +880,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
+ .exceptionally(e -> {
+ String msg = format("failed to update namespace policies [%s], "
+ + "NamespaceBundle: %s due to %s",
+ nsname.toString(), bundle.getBundleRange(), e.getMessage());
+ LOG.warn(msg);
+ updateFuture.completeExceptionally(
Review Comment:
I wonder what will happen if `updateFuture` completeExceptionally here and complete success in L896(after updateNamespaceBundles success). There is no executing order guarantee for these two ops.
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r983318634
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
After the split, several zookeeper paths are updated or created:
1. Each bundle corresponds to the path of the owner broker;
2. Update the local policies path;
Do you want to update the operations of these paths into atomic operations?
--
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] lordcheng10 commented on a diff in pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on code in PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#discussion_r985103507
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -880,12 +881,22 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
for (NamespaceBundle sBundle : splittedBundles.getRight()) {
checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
}
+ updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())
Review Comment:
Modify the execution order of the two methods to ensure that updateNamespaceBundlesForPolicies is executed after updateNamespaceBundles is completed.
@mattisonchao PTAL,thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on PR #17797:
URL: https://github.com/apache/pulsar/pull/17797#issuecomment-1272251411
/pulsarbot run-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
[GitHub] [pulsar] AnonHxy merged pull request #17797: [fix][broker] Update new bundle-range to policies after bundle split
Posted by GitBox <gi...@apache.org>.
AnonHxy merged PR #17797:
URL: https://github.com/apache/pulsar/pull/17797
--
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