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/02/01 14:50:40 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request #14087: Make ``updateDynamicConfiguration`` pure async.
mattisonchao opened a new pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087
### Motivation
Make ``BrokerBase#updateDynamicConfiguration`` to pure async.
### Modifications
- change sync method to async.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
Check the box below or label this PR directly (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800358980
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
fixed, 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] mattisonchao commented on pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#issuecomment-1027508028
/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] nodece commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800332200
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
You can improve `AdminResource#resumeAsyncResponseExceptionally()` and move to `PulsarWebResource`, so like:
```java
protected static void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
throwable = FutureUtil.unwrapCompletionException(throwable);
if (throwable instanceof WebApplicationException) {
asyncResponse.resume(throwable);
} else if (throwable instanceof BrokerServiceException.NotAllowedException) {
asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
} else {
asyncResponse.resume(new RestException(throwable));
}
}
```
We shouldn't have two similar methods, it makes confusion.
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800295072
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -235,31 +244,24 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
* @param configValue
* : configuration value
*/
- private synchronized void persistDynamicConfiguration(String configName, String configValue) {
- try {
- if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
- throw new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value");
- }
- if (BrokerService.isDynamicConfiguration(configName)) {
- dynamicConfigurationResources().setDynamicConfigurationWithCreate(old -> {
- Map<String, String> configurationMap = old.isPresent() ? old.get() : Maps.newHashMap();
- configurationMap.put(configName, configValue);
- return configurationMap;
- });
- LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
- configValue);
- }
- throw new RestException(Status.PRECONDITION_FAILED, " Can't update non-dynamic configuration");
+ private synchronized CompletableFuture<Void> persistDynamicConfiguration(String configName, String configValue) {
+ if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
+ return FutureUtil
+ .failedFuture(new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value"));
+ }
+ if (BrokerService.isDynamicConfiguration(configName)) {
+ return dynamicConfigurationResources().setDynamicConfigurationWithCreateAsync(old -> {
+ Map<String, String> configurationMap = old.orElseGet(Maps::newHashMap);
+ configurationMap.put(configName, configValue);
+ return configurationMap;
+ });
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
+ configValue);
}
- } catch (RestException re) {
- throw re;
- } catch (Exception ie) {
- LOG.error("[{}] Failed to update configuration {}/{}, {}", clientAppId(), configName, configValue,
- ie.getMessage(), ie);
- throw new RestException(ie);
+ return FutureUtil.failedFuture(new RestException(Status.PRECONDITION_FAILED,
+ " Can't update non-dynamic configuration"));
Review comment:
```suggestion
"Cannot update non-dynamic configuration"));
```
--
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] lhotari commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800691821
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -755,13 +755,14 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
return future;
}
- protected void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
- if (throwable instanceof WebApplicationException) {
- asyncResponse.resume(throwable);
- } else if (throwable instanceof BrokerServiceException.NotAllowedException) {
- asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
+ protected static void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable exception) {
+ Throwable realCause = FutureUtil.unwrapCompletionException(exception);
+ if (realCause instanceof WebApplicationException) {
+ asyncResponse.resume(realCause);
+ } else if (realCause instanceof BrokerServiceException.NotAllowedException) {
+ asyncResponse.resume(new RestException(Status.CONFLICT, realCause));
Review comment:
> I think maybe we need to normalize exceptions and HTTP code after refactoring the admin side async code.
yes, that makes sense to handle it separately.
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800293940
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -168,10 +168,19 @@ public void getLeaderBroker(@Suspended final AsyncResponse asyncResponse) {
@ApiResponse(code = 404, message = "Configuration not found"),
@ApiResponse(code = 412, message = "Invalid dynamic-config value"),
@ApiResponse(code = 500, message = "Internal server error") })
- public void updateDynamicConfiguration(@PathParam("configName") String configName,
- @PathParam("configValue") String configValue) throws Exception {
- validateSuperUserAccess();
- persistDynamicConfiguration(configName, configValue);
+ public void updateDynamicConfiguration(@Suspended AsyncResponse asyncResponse,
+ @PathParam("configName") String configName,
+ @PathParam("configValue") String configValue) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> persistDynamicConfiguration(configName, configValue))
+ .thenAccept(__ -> {
+ LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
+ asyncResponse.resume(Response.ok().build());
+ }).exceptionally(ex -> {
+ LOG.error("[{}] Failed to update configuration {}/{}, {}", clientAppId(), configName, configValue,
Review comment:
```suggestion
LOG.error("[{}] Failed to update configuration {}/{}", clientAppId(), configName, configValue, ex);
```
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800473305
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -235,31 +242,25 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
* @param configValue
* : configuration value
*/
- private synchronized void persistDynamicConfiguration(String configName, String configValue) {
- try {
- if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
- throw new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value");
- }
- if (BrokerService.isDynamicConfiguration(configName)) {
- dynamicConfigurationResources().setDynamicConfigurationWithCreate(old -> {
- Map<String, String> configurationMap = old.isPresent() ? old.get() : Maps.newHashMap();
- configurationMap.put(configName, configValue);
- return configurationMap;
- });
- LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
- configValue);
- }
- throw new RestException(Status.PRECONDITION_FAILED, " Can't update non-dynamic configuration");
+ private synchronized CompletableFuture<Void> persistDynamicConfigurationAsync(
+ String configName, String configValue) {
+ if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
+ return FutureUtil
+ .failedFuture(new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value"));
+ }
+ if (BrokerService.isDynamicConfiguration(configName)) {
+ return dynamicConfigurationResources().setDynamicConfigurationWithCreateAsync(old -> {
+ Map<String, String> configurationMap = old.orElseGet(Maps::newHashMap);
+ configurationMap.put(configName, configValue);
+ return configurationMap;
+ });
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("[{}] Cannot update non-dynamic configuration {}/{}", clientAppId(), configName,
+ configValue);
}
Review comment:
```suggestion
```
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800686352
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -755,13 +755,14 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
return future;
}
- protected void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
- if (throwable instanceof WebApplicationException) {
- asyncResponse.resume(throwable);
- } else if (throwable instanceof BrokerServiceException.NotAllowedException) {
- asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
+ protected static void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable exception) {
+ Throwable realCause = FutureUtil.unwrapCompletionException(exception);
+ if (realCause instanceof WebApplicationException) {
+ asyncResponse.resume(realCause);
+ } else if (realCause instanceof BrokerServiceException.NotAllowedException) {
+ asyncResponse.resume(new RestException(Status.CONFLICT, realCause));
Review comment:
Because the ``BrokerServiceException.NotAllowedException``thrown by some state conflict and others reason.
well, I think maybe we need to normalize exceptions and HTTP code after refactoring the admin side async code.
Thanks for your suggestion, let me know what you 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] nodece commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800295211
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -235,31 +244,24 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
* @param configValue
* : configuration value
*/
- private synchronized void persistDynamicConfiguration(String configName, String configValue) {
- try {
- if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
- throw new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value");
- }
- if (BrokerService.isDynamicConfiguration(configName)) {
- dynamicConfigurationResources().setDynamicConfigurationWithCreate(old -> {
- Map<String, String> configurationMap = old.isPresent() ? old.get() : Maps.newHashMap();
- configurationMap.put(configName, configValue);
- return configurationMap;
- });
- LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
- configValue);
- }
- throw new RestException(Status.PRECONDITION_FAILED, " Can't update non-dynamic configuration");
+ private synchronized CompletableFuture<Void> persistDynamicConfiguration(String configName, String configValue) {
+ if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
+ return FutureUtil
+ .failedFuture(new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value"));
+ }
+ if (BrokerService.isDynamicConfiguration(configName)) {
+ return dynamicConfigurationResources().setDynamicConfigurationWithCreateAsync(old -> {
+ Map<String, String> configurationMap = old.orElseGet(Maps::newHashMap);
+ configurationMap.put(configName, configValue);
+ return configurationMap;
+ });
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
Review comment:
```suggestion
LOG.debug("[{}] Cannot update non-dynamic configuration {}/{}", clientAppId(), configName,
```
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800296458
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
I think you can use `resumeAsyncResponseExceptionally`.
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800472595
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -235,31 +242,25 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
* @param configValue
* : configuration value
*/
- private synchronized void persistDynamicConfiguration(String configName, String configValue) {
- try {
- if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
- throw new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value");
- }
- if (BrokerService.isDynamicConfiguration(configName)) {
- dynamicConfigurationResources().setDynamicConfigurationWithCreate(old -> {
- Map<String, String> configurationMap = old.isPresent() ? old.get() : Maps.newHashMap();
- configurationMap.put(configName, configValue);
- return configurationMap;
- });
- LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
- configValue);
- }
- throw new RestException(Status.PRECONDITION_FAILED, " Can't update non-dynamic configuration");
+ private synchronized CompletableFuture<Void> persistDynamicConfigurationAsync(
+ String configName, String configValue) {
+ if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
+ return FutureUtil
+ .failedFuture(new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value"));
+ }
+ if (BrokerService.isDynamicConfiguration(configName)) {
+ return dynamicConfigurationResources().setDynamicConfigurationWithCreateAsync(old -> {
+ Map<String, String> configurationMap = old.orElseGet(Maps::newHashMap);
+ configurationMap.put(configName, configValue);
+ return configurationMap;
+ });
+ } else {
+ if (LOG.isDebugEnabled()) {
Review comment:
Sure, thanks to you.
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800323226
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
I think we need to use "handleCommonRestAsyncException" instead of "resumeAsyncResponseExceptionally".
Because using ``handleCommonRestAsyncException`` we don't need to care if it's a ``CompletionException`` exception. And we don't need to "return null" in "exceptionally". I think this change could be clearer.
Thanks for your review, and let me know what you think.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
I think we need to use ``handleCommonRestAsyncException`` instead of "resumeAsyncResponseExceptionally".
Because using ``handleCommonRestAsyncException`` we don't need to care if it's a ``CompletionException`` exception. And we don't need to "return null" in "exceptionally". I think this change could be clearer.
Thanks for your review, and let me know what you think.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
I think we need to use ``handleCommonRestAsyncException`` instead of ``resumeAsyncResponseExceptionally``.
Because using ``handleCommonRestAsyncException`` we don't need to care if it's a ``CompletionException`` exception. And we don't need to "return null" in "exceptionally". I think this change could be clearer.
Thanks for your review, and let me know what you 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] nodece commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800332200
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
You can improve `AdminResource#resumeAsyncResponseExceptionally()` and move to `PulsarWebResource`, so like:
```java
protected static void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
throwable = FutureUtil.unwrapCompletionException(throwable);
if (throwable instanceof WebApplicationException) {
asyncResponse.resume(throwable);
} else if (throwable instanceof BrokerServiceException.NotAllowedException) {
asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
} else {
asyncResponse.resume(new RestException(throwable));
}
}
```
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800471905
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -235,31 +242,25 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
* @param configValue
* : configuration value
*/
- private synchronized void persistDynamicConfiguration(String configName, String configValue) {
- try {
- if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
- throw new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value");
- }
- if (BrokerService.isDynamicConfiguration(configName)) {
- dynamicConfigurationResources().setDynamicConfigurationWithCreate(old -> {
- Map<String, String> configurationMap = old.isPresent() ? old.get() : Maps.newHashMap();
- configurationMap.put(configName, configValue);
- return configurationMap;
- });
- LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
- configValue);
- }
- throw new RestException(Status.PRECONDITION_FAILED, " Can't update non-dynamic configuration");
+ private synchronized CompletableFuture<Void> persistDynamicConfigurationAsync(
+ String configName, String configValue) {
+ if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
+ return FutureUtil
+ .failedFuture(new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value"));
+ }
+ if (BrokerService.isDynamicConfiguration(configName)) {
+ return dynamicConfigurationResources().setDynamicConfigurationWithCreateAsync(old -> {
+ Map<String, String> configurationMap = old.orElseGet(Maps::newHashMap);
+ configurationMap.put(configName, configValue);
+ return configurationMap;
+ });
+ } else {
+ if (LOG.isDebugEnabled()) {
Review comment:
This debug log seems not necessary. We have error log in `updateDynamicConfiguration`.
--
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 pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#issuecomment-1031411923
@codelipenghui @eolivelli @lhotari 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] mattisonchao commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800375366
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -755,14 +755,16 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
return future;
}
- protected void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
- if (throwable instanceof WebApplicationException) {
- asyncResponse.resume(throwable);
- } else if (throwable instanceof BrokerServiceException.NotAllowedException) {
- asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
+ protected static Void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable exception) {
Review comment:
Could we use it in ``exceptionally`` to avoid unnecessary ``return null``?
Use ``Avoid`` return type
```java
//....
.exceptionally(ex -> {
return handleCommonRestAsyncException(asyncResponse, ex);
})
```
Don't use ``Avoid`` return type
```java
//....
.exceptionally(ex -> {
handleCommonRestAsyncException(asyncResponse, ex);
return null;
})
```
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800375366
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -755,14 +755,16 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
return future;
}
- protected void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
- if (throwable instanceof WebApplicationException) {
- asyncResponse.resume(throwable);
- } else if (throwable instanceof BrokerServiceException.NotAllowedException) {
- asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
+ protected static Void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable exception) {
Review comment:
Could we use it in ``exceptionally`` to avoid unnecessary ``return null``?
Use ``Avoid`` return type
```java
//....
.exceptionally(ex -> {
return resumeAsyncResponseExceptionally(asyncResponse, ex);
})
```
Don't use ``Avoid`` return type
```java
//....
.exceptionally(ex -> {
resumeAsyncResponseExceptionally(asyncResponse, ex);
return null;
})
```
--
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] RobertIndie commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800270052
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -235,31 +244,24 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
* @param configValue
* : configuration value
*/
- private synchronized void persistDynamicConfiguration(String configName, String configValue) {
- try {
- if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
- throw new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value");
- }
- if (BrokerService.isDynamicConfiguration(configName)) {
- dynamicConfigurationResources().setDynamicConfigurationWithCreate(old -> {
- Map<String, String> configurationMap = old.isPresent() ? old.get() : Maps.newHashMap();
- configurationMap.put(configName, configValue);
- return configurationMap;
- });
- LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
- configValue);
- }
- throw new RestException(Status.PRECONDITION_FAILED, " Can't update non-dynamic configuration");
+ private synchronized CompletableFuture<Void> persistDynamicConfiguration(String configName, String configValue) {
Review comment:
```suggestion
private synchronized CompletableFuture<Void> persistDynamicConfigurationAsync(String configName, String configValue) {
```
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800372326
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -755,14 +755,16 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
return future;
}
- protected void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
- if (throwable instanceof WebApplicationException) {
- asyncResponse.resume(throwable);
- } else if (throwable instanceof BrokerServiceException.NotAllowedException) {
- asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
+ protected static Void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable exception) {
Review comment:
Why return the `Void` type? It looks meaningless.
--
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 pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#issuecomment-1031240369
/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] lhotari merged pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800296458
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -1166,7 +1166,7 @@ public void validateTopicOperation(TopicName topicName, TopicOperation operation
}
}
- protected Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
+ protected static Void handleCommonRestAsyncException(AsyncResponse asyncResponse, Throwable ex) {
Review comment:
I think you can use `AdminResource#resumeAsyncResponseExceptionally()` instead.
--
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] lhotari commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800691821
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -755,13 +755,14 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
return future;
}
- protected void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
- if (throwable instanceof WebApplicationException) {
- asyncResponse.resume(throwable);
- } else if (throwable instanceof BrokerServiceException.NotAllowedException) {
- asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
+ protected static void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable exception) {
+ Throwable realCause = FutureUtil.unwrapCompletionException(exception);
+ if (realCause instanceof WebApplicationException) {
+ asyncResponse.resume(realCause);
+ } else if (realCause instanceof BrokerServiceException.NotAllowedException) {
+ asyncResponse.resume(new RestException(Status.CONFLICT, realCause));
Review comment:
> I think maybe we need to normalize exceptions and HTTP code after refactoring the admin side async code.
yes, that makes sense to handle it separately.
--
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 pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#issuecomment-1030985889
@codelipenghui @eolivelli @Technoboy- @Jason918 @RobertIndie @nodece
PTAL, when you have time. :)
--
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 change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800295072
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java
##########
@@ -235,31 +244,24 @@ public void deleteDynamicConfiguration(@PathParam("configName") String configNam
* @param configValue
* : configuration value
*/
- private synchronized void persistDynamicConfiguration(String configName, String configValue) {
- try {
- if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
- throw new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value");
- }
- if (BrokerService.isDynamicConfiguration(configName)) {
- dynamicConfigurationResources().setDynamicConfigurationWithCreate(old -> {
- Map<String, String> configurationMap = old.isPresent() ? old.get() : Maps.newHashMap();
- configurationMap.put(configName, configValue);
- return configurationMap;
- });
- LOG.info("[{}] Updated Service configuration {}/{}", clientAppId(), configName, configValue);
- } else {
- if (LOG.isDebugEnabled()) {
- LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
- configValue);
- }
- throw new RestException(Status.PRECONDITION_FAILED, " Can't update non-dynamic configuration");
+ private synchronized CompletableFuture<Void> persistDynamicConfiguration(String configName, String configValue) {
+ if (!BrokerService.validateDynamicConfiguration(configName, configValue)) {
+ return FutureUtil
+ .failedFuture(new RestException(Status.PRECONDITION_FAILED, " Invalid dynamic-config value"));
+ }
+ if (BrokerService.isDynamicConfiguration(configName)) {
+ return dynamicConfigurationResources().setDynamicConfigurationWithCreateAsync(old -> {
+ Map<String, String> configurationMap = old.orElseGet(Maps::newHashMap);
+ configurationMap.put(configName, configValue);
+ return configurationMap;
+ });
+ } else {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("[{}] Can't update non-dynamic configuration {}/{}", clientAppId(), configName,
+ configValue);
}
- } catch (RestException re) {
- throw re;
- } catch (Exception ie) {
- LOG.error("[{}] Failed to update configuration {}/{}, {}", clientAppId(), configName, configValue,
- ie.getMessage(), ie);
- throw new RestException(ie);
+ return FutureUtil.failedFuture(new RestException(Status.PRECONDITION_FAILED,
+ " Can't update non-dynamic configuration"));
Review comment:
```suggestion
"Can't update non-dynamic configuration"));
```
--
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] lhotari commented on a change in pull request #14087: Make ``BrokerBase#updateDynamicConfiguration`` pure async.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14087:
URL: https://github.com/apache/pulsar/pull/14087#discussion_r800634261
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -755,13 +755,14 @@ protected void internalCreatePartitionedTopic(AsyncResponse asyncResponse, int n
return future;
}
- protected void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable throwable) {
- if (throwable instanceof WebApplicationException) {
- asyncResponse.resume(throwable);
- } else if (throwable instanceof BrokerServiceException.NotAllowedException) {
- asyncResponse.resume(new RestException(Status.CONFLICT, throwable));
+ protected static void resumeAsyncResponseExceptionally(AsyncResponse asyncResponse, Throwable exception) {
+ Throwable realCause = FutureUtil.unwrapCompletionException(exception);
+ if (realCause instanceof WebApplicationException) {
+ asyncResponse.resume(realCause);
+ } else if (realCause instanceof BrokerServiceException.NotAllowedException) {
+ asyncResponse.resume(new RestException(Status.CONFLICT, realCause));
Review comment:
Shouldn't this be `Status.FORBIDDEN` ? This change was already made in #13745, but just wondering.
--
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