You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "mattisonchao (via GitHub)" <gi...@apache.org> on 2023/10/29 09:40:06 UTC
[PR] [improve][broker] Refactor method BrokerService#unloadServiceUnit [pulsar]
mattisonchao opened a new pull request, #21463:
URL: https://github.com/apache/pulsar/pull/21463
### Motivation
Refactor method BrokerService#unloadServiceUnit to improve exception handling, logging and method call.
### Modifications
- Add parameters NPE check
- Ignore the exception of `topicCreationFuture`
> We should ignore the exception from topic creation future. It will give the caller a precise exception related to topic closing.
- throw `TimeoutException` when the method running timeout.
> As a public method, we should let users decide what they want to do when the method runs timeout. Plus, I checked all the usage of this method and all of them ignore the exception directly. It shouldn't be a breaking change.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] Refactor method BrokerService#unloadServiceUnit [pulsar]
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao closed pull request #21463: [improve][broker] Refactor method BrokerService#unloadServiceUnit
URL: https://github.com/apache/pulsar/pull/21463
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] Refactor method BrokerService#unloadServiceUnit [pulsar]
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao commented on code in PR #21463:
URL: https://github.com/apache/pulsar/pull/21463#discussion_r1376119785
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2189,70 +2188,53 @@ public CompletableFuture<Void> checkTopicNsOwnership(final String topic) {
});
}
- public CompletableFuture<Integer> unloadServiceUnit(NamespaceBundle serviceUnit,
- boolean closeWithoutWaitingClientDisconnect, long timeout, TimeUnit unit) {
- CompletableFuture<Integer> future = unloadServiceUnit(serviceUnit, closeWithoutWaitingClientDisconnect);
- ScheduledFuture<?> taskTimeout = executor().schedule(() -> {
- if (!future.isDone()) {
- log.warn("Unloading of {} has timed out", serviceUnit);
- // Complete the future with no error
- future.complete(0);
- }
- }, timeout, unit);
-
- future.whenComplete((r, ex) -> taskTimeout.cancel(true));
- return future;
- }
-
/**
- * Unload all the topic served by the broker service under the given service unit.
+ * Unload and close topics by namespace-bundle. It will close all topics in the namespace bundle.
*
- * @param serviceUnit
- * @param closeWithoutWaitingClientDisconnect don't wait for clients to disconnect
- * and forcefully close managed-ledger
- * @return
+ * @param namespaceBundle Namespace bundle
+ * @param closeWithoutWaitingClientDisconnect Whether wait client disconnection while closing topic
+ * @param timeout Timeout time
+ * @param unit Timeout time unit
+ * @return Return the future of namespace bundle unloading and how many topics has been unloaded.
+ * Note:
+ * If
+ * @throws NullPointerException Throw NPE when namespace-bundle or time unit is null.
*/
- private CompletableFuture<Integer> unloadServiceUnit(NamespaceBundle serviceUnit,
- boolean closeWithoutWaitingClientDisconnect) {
- List<CompletableFuture<Void>> closeFutures = new ArrayList<>();
+ public CompletableFuture<Integer> unloadServiceUnit(NamespaceBundle namespaceBundle,
+ boolean closeWithoutWaitingClientDisconnect, long timeout, TimeUnit unit) {
+ requireNonNull(namespaceBundle);
Review Comment:
Just a simple NPE check for public API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] Refactor method BrokerService#unloadServiceUnit [pulsar]
Posted by "mattisonchao (via GitHub)" <gi...@apache.org>.
mattisonchao closed pull request #21463: [improve][broker] Refactor method BrokerService#unloadServiceUnit
URL: https://github.com/apache/pulsar/pull/21463
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][broker] Refactor method BrokerService#unloadServiceUnit [pulsar]
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #21463:
URL: https://github.com/apache/pulsar/pull/21463#discussion_r1375692002
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2189,70 +2188,53 @@ public CompletableFuture<Void> checkTopicNsOwnership(final String topic) {
});
}
- public CompletableFuture<Integer> unloadServiceUnit(NamespaceBundle serviceUnit,
- boolean closeWithoutWaitingClientDisconnect, long timeout, TimeUnit unit) {
- CompletableFuture<Integer> future = unloadServiceUnit(serviceUnit, closeWithoutWaitingClientDisconnect);
- ScheduledFuture<?> taskTimeout = executor().schedule(() -> {
- if (!future.isDone()) {
- log.warn("Unloading of {} has timed out", serviceUnit);
- // Complete the future with no error
- future.complete(0);
- }
- }, timeout, unit);
-
- future.whenComplete((r, ex) -> taskTimeout.cancel(true));
- return future;
- }
-
/**
- * Unload all the topic served by the broker service under the given service unit.
+ * Unload and close topics by namespace-bundle. It will close all topics in the namespace bundle.
*
- * @param serviceUnit
- * @param closeWithoutWaitingClientDisconnect don't wait for clients to disconnect
- * and forcefully close managed-ledger
- * @return
+ * @param namespaceBundle Namespace bundle
+ * @param closeWithoutWaitingClientDisconnect Whether wait client disconnection while closing topic
+ * @param timeout Timeout time
+ * @param unit Timeout time unit
+ * @return Return the future of namespace bundle unloading and how many topics has been unloaded.
+ * Note:
+ * If
+ * @throws NullPointerException Throw NPE when namespace-bundle or time unit is null.
*/
- private CompletableFuture<Integer> unloadServiceUnit(NamespaceBundle serviceUnit,
- boolean closeWithoutWaitingClientDisconnect) {
- List<CompletableFuture<Void>> closeFutures = new ArrayList<>();
+ public CompletableFuture<Integer> unloadServiceUnit(NamespaceBundle namespaceBundle,
+ boolean closeWithoutWaitingClientDisconnect, long timeout, TimeUnit unit) {
+ requireNonNull(namespaceBundle);
Review Comment:
I have a question. Why we need check non-null here for `namespaceBundle` and `unit`, is it necessory :) ?
--
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