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/08/22 08:10:09 UTC
[GitHub] [pulsar] Pomelongan opened a new pull request, #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Pomelongan opened a new pull request, #17206:
URL: https://github.com/apache/pulsar/pull/17206
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
### Motivation
See https://github.com/apache/pulsar/issues/14365
### Verifying this change
Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [x] `doc-not-needed`
--
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] nodece commented on a diff in pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r953271930
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1572,6 +1572,19 @@ private void doUpdatePersistence(PersistencePolicies persistence) {
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsync(PersistencePolicies persistence) {
+ return updatePoliciesAsync(namespaceName, policies -> {
+ policies.persistence = persistence;
+ return policies;
+ }).thenAccept(__ -> log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}",
+ clientAppId(), namespaceName, persistence)
+ ).exceptionally(ex -> {
+ log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
+ ex);
+ throw new RestException(ex);
Review Comment:
You can move log logic to v2.
--
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 #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r951309253
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1559,19 +1560,37 @@ protected void internalSetPersistence(PersistencePolicies persistence) {
private void doUpdatePersistence(PersistencePolicies persistence) {
try {
- updatePolicies(namespaceName, policies -> {
- policies.persistence = persistence;
- return policies;
- });
- log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
- namespaceName, jsonMapper().writeValueAsString(persistence));
+ doUpdatePersistenceAsyn(persistence).get(namespaceResources().getOperationTimeoutSec(), TimeUnit.SECONDS);
Review Comment:
I think the modification of this method is unrelated to this PR, we'd better do not change it 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] Pomelongan commented on pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#issuecomment-1229453375
/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] Pomelongan commented on pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#issuecomment-1228244183
/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] Pomelongan commented on a diff in pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r955831360
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1572,6 +1572,19 @@ private void doUpdatePersistence(PersistencePolicies persistence) {
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsync(PersistencePolicies persistence) {
+ return updatePoliciesAsync(namespaceName, policies -> {
+ policies.persistence = persistence;
+ return policies;
+ }).thenAccept(__ -> log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}",
+ clientAppId(), namespaceName, persistence)
+ ).exceptionally(ex -> {
+ log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
+ ex);
+ throw new RestException(ex);
Review Comment:
PTAL @nodece
--
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] Pomelongan commented on a diff in pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r951435902
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1543,10 +1544,10 @@ protected void internalSetRetention(RetentionPolicies retention) {
}
}
- protected void internalDeletePersistence() {
- validateNamespacePolicyOperation(namespaceName, PolicyName.PERSISTENCE, PolicyOperation.WRITE);
- validatePoliciesReadOnlyAccess();
- doUpdatePersistence(null);
+ protected CompletableFuture<Void> internalDeletePersistenceAsyn() {
Review Comment:
Thank you for your review. I have revised it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Pomelongan commented on pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#issuecomment-1229395954
/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
[GitHub] [pulsar] Pomelongan commented on pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#issuecomment-1229462509
/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] Pomelongan commented on a diff in pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r955826472
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1572,6 +1572,19 @@ private void doUpdatePersistence(PersistencePolicies persistence) {
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsync(PersistencePolicies persistence) {
+ return updatePoliciesAsync(namespaceName, policies -> {
+ policies.persistence = persistence;
+ return policies;
+ }).thenAccept(__ -> log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}",
+ clientAppId(), namespaceName, persistence)
+ ).exceptionally(ex -> {
+ log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
+ ex);
+ throw new RestException(ex);
Review Comment:
> You can move log logic to v2.
PTAL
--
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] Pomelongan commented on pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#issuecomment-1229382407
/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] Pomelongan commented on pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#issuecomment-1229433466
/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] gaozhangmin merged pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
gaozhangmin merged PR #17206:
URL: https://github.com/apache/pulsar/pull/17206
--
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] Pomelongan commented on a diff in pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r951436122
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1559,19 +1560,37 @@ protected void internalSetPersistence(PersistencePolicies persistence) {
private void doUpdatePersistence(PersistencePolicies persistence) {
try {
- updatePolicies(namespaceName, policies -> {
- policies.persistence = persistence;
- return policies;
- });
- log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
- namespaceName, jsonMapper().writeValueAsString(persistence));
+ doUpdatePersistenceAsyn(persistence).get(namespaceResources().getOperationTimeoutSec(), TimeUnit.SECONDS);
} catch (Exception e) {
- log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
- e);
- throw new RestException(e);
+ Throwable cause = e.getCause();
+ if (!(cause instanceof RestException)) {
+ throw new RestException(cause);
+ } else {
+ throw (RestException) cause;
+ }
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsyn(PersistencePolicies persistence) {
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ updatePoliciesAsync(namespaceName, policies -> {
+ policies.persistence = persistence;
+ return policies;
+ }).thenAccept(v -> {
+ try {
+ log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
+ namespaceName, jsonMapper().writeValueAsString(persistence));
+ } catch (JsonProcessingException ignore) {}
Review Comment:
Thank you for your review. I have revised it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Pomelongan commented on a diff in pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r951436409
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1559,19 +1560,37 @@ protected void internalSetPersistence(PersistencePolicies persistence) {
private void doUpdatePersistence(PersistencePolicies persistence) {
try {
- updatePolicies(namespaceName, policies -> {
- policies.persistence = persistence;
- return policies;
- });
- log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
- namespaceName, jsonMapper().writeValueAsString(persistence));
+ doUpdatePersistenceAsyn(persistence).get(namespaceResources().getOperationTimeoutSec(), TimeUnit.SECONDS);
} catch (Exception e) {
- log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
- e);
- throw new RestException(e);
+ Throwable cause = e.getCause();
+ if (!(cause instanceof RestException)) {
+ throw new RestException(cause);
+ } else {
+ throw (RestException) cause;
+ }
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsyn(PersistencePolicies persistence) {
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ updatePoliciesAsync(namespaceName, policies -> {
Review Comment:
Thank you for your review. I have revised it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] AnonHxy commented on a diff in pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r951288419
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1559,19 +1560,37 @@ protected void internalSetPersistence(PersistencePolicies persistence) {
private void doUpdatePersistence(PersistencePolicies persistence) {
try {
- updatePolicies(namespaceName, policies -> {
- policies.persistence = persistence;
- return policies;
- });
- log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
- namespaceName, jsonMapper().writeValueAsString(persistence));
+ doUpdatePersistenceAsyn(persistence).get(namespaceResources().getOperationTimeoutSec(), TimeUnit.SECONDS);
} catch (Exception e) {
- log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
- e);
- throw new RestException(e);
+ Throwable cause = e.getCause();
+ if (!(cause instanceof RestException)) {
+ throw new RestException(cause);
+ } else {
+ throw (RestException) cause;
+ }
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsyn(PersistencePolicies persistence) {
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ updatePoliciesAsync(namespaceName, policies -> {
+ policies.persistence = persistence;
+ return policies;
+ }).thenAccept(v -> {
+ try {
+ log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
+ namespaceName, jsonMapper().writeValueAsString(persistence));
+ } catch (JsonProcessingException ignore) {}
Review Comment:
```suggestion
} catch (Exception ignore) {}
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1543,10 +1544,10 @@ protected void internalSetRetention(RetentionPolicies retention) {
}
}
- protected void internalDeletePersistence() {
- validateNamespacePolicyOperation(namespaceName, PolicyName.PERSISTENCE, PolicyOperation.WRITE);
- validatePoliciesReadOnlyAccess();
- doUpdatePersistence(null);
+ protected CompletableFuture<Void> internalDeletePersistenceAsyn() {
Review Comment:
```suggestion
protected CompletableFuture<Void> internalDeletePersistenceAsync() {
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1559,19 +1560,37 @@ protected void internalSetPersistence(PersistencePolicies persistence) {
private void doUpdatePersistence(PersistencePolicies persistence) {
try {
- updatePolicies(namespaceName, policies -> {
- policies.persistence = persistence;
- return policies;
- });
- log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
- namespaceName, jsonMapper().writeValueAsString(persistence));
+ doUpdatePersistenceAsyn(persistence).get(namespaceResources().getOperationTimeoutSec(), TimeUnit.SECONDS);
Review Comment:
I thinks the modification of this method is unrelated to this PR, we'd better do not change it now
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1559,19 +1560,37 @@ protected void internalSetPersistence(PersistencePolicies persistence) {
private void doUpdatePersistence(PersistencePolicies persistence) {
try {
- updatePolicies(namespaceName, policies -> {
- policies.persistence = persistence;
- return policies;
- });
- log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}", clientAppId(),
- namespaceName, jsonMapper().writeValueAsString(persistence));
+ doUpdatePersistenceAsyn(persistence).get(namespaceResources().getOperationTimeoutSec(), TimeUnit.SECONDS);
} catch (Exception e) {
- log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
- e);
- throw new RestException(e);
+ Throwable cause = e.getCause();
+ if (!(cause instanceof RestException)) {
+ throw new RestException(cause);
+ } else {
+ throw (RestException) cause;
+ }
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsyn(PersistencePolicies persistence) {
+ CompletableFuture<Void> result = new CompletableFuture<>();
+ updatePoliciesAsync(namespaceName, policies -> {
Review Comment:
```suggestion
return updatePoliciesAsync(namespaceName, policies -> {
```
It's better return directly I think :)
--
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 #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#discussion_r951574585
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java:
##########
@@ -1572,6 +1572,19 @@ private void doUpdatePersistence(PersistencePolicies persistence) {
}
}
+ private CompletableFuture<Void> doUpdatePersistenceAsync(PersistencePolicies persistence) {
+ return updatePoliciesAsync(namespaceName, policies -> {
+ policies.persistence = persistence;
+ return policies;
+ }).thenAccept(__ -> log.info("[{}] Successfully updated persistence configuration: namespace={}, map={}",
+ clientAppId(), namespaceName, persistence)
+ ).exceptionally(ex -> {
+ log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
+ ex);
+ throw new RestException(ex);
Review Comment:
```suggestion
Throwable cause = FutureUtil.unwrapCompletionException(ex);
log.error("[{}] Failed to update persistence configuration for namespace {}", clientAppId(), namespaceName,
cause);
throw new RestException(cause);
```
--
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] HQebupt commented on pull request #17206: [improve][broker][PIP-149]make deletePersistence method async in Namespaces
Posted by GitBox <gi...@apache.org>.
HQebupt commented on PR #17206:
URL: https://github.com/apache/pulsar/pull/17206#issuecomment-1229405274
@Pomelongan Please fix CI failure.
--
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