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/04/02 01:44:29 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Technoboy- opened a new pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876
### Motivation
When topic does not exist, the original logic will return `TEMPORARY_REDIRECT`, it will cause the client below exception :
```
Caused by: java.lang.NullPointerException: originalUrl
at org.asynchttpclient.util.Assertions.assertNotNull(Assertions.java:23)
at org.asynchttpclient.uri.UriParser.parse(UriParser.java:344)
at org.asynchttpclient.uri.Uri.create(Uri.java:67)
at org.asynchttpclient.netty.handler.intercept.Redirect30xInterceptor.exitAfterHandlingRedirect(Redirect30xInterceptor.java:128)
at org.asynchttpclient.netty.handler.intercept.Interceptors.exitAfterIntercept(Interceptors.java:98)
at org.asynchttpclient.netty.handler.HttpHandler.handleHttpResponse(HttpHandler.java:78)
at org.asynchttpclient.netty.handler.HttpHandler.handleRead(HttpHandler.java:140)
```
### Modification
- Return `NOT_FOUND` instead of `TEMPORARY_REDIRECT`.
- Make `internalGetPendingAckInternalStats` fully async.
### Verifying this change
The current AdminApiTransactionTest has covered this patch.
### Documentation
- [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] Technoboy- closed pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876
--
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] Technoboy- commented on pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#issuecomment-1080194081
> Please add some test case
Hi @eolivelli , the current AdminApiTransactionTest has covered this patch.
--
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] Technoboy- commented on pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#issuecomment-1080276402
> @Technoboy- can we add some assertions on test?
Updated. Add for `Topic Not Found`.
--
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] Technoboy- commented on a change in pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r838152524
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -222,7 +228,24 @@ public void getPendingAckInternalStats(@Suspended final AsyncResponse asyncRespo
@PathParam("topic") @Encoded String encodedTopic,
@PathParam("subName") String subName,
@QueryParam("metadata") @DefaultValue("false") boolean metadata) {
- internalGetPendingAckInternalStats(asyncResponse, authoritative,
- TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata);
+
+ internalGetPendingAckInternalStats(authoritative,
+ TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata)
+ .thenAccept(stats -> asyncResponse.resume(stats))
Review comment:
Success log seems useless.
--
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] Technoboy- commented on pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#issuecomment-1079945675
> Please add some test case
Hi @eolivelli. The original issue is caused by missing using `TEMPORARY_REDIRECT `. I have replaced this wrong status . so the test could not verify. And the existing test has covered the current logic.
--
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] Technoboy- commented on pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#issuecomment-1080277394
> Please add some test case
Add assertions related to the modification.
--
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] Technoboy- commented on a change in pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r838150927
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java
##########
@@ -373,6 +375,23 @@ public void testGetPendingAckInternalStats() throws Exception {
TransactionImpl transaction = (TransactionImpl) getTransaction();
final String topic = "persistent://public/default/testGetPendingAckInternalStats";
final String subName = "test";
+ try {
+ admin.transactions()
+ .getPendingAckInternalStatsAsync(topic, subName, true).get();
Review comment:
yes, fixed
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java
##########
@@ -373,6 +375,23 @@ public void testGetPendingAckInternalStats() throws Exception {
TransactionImpl transaction = (TransactionImpl) getTransaction();
final String topic = "persistent://public/default/testGetPendingAckInternalStats";
final String subName = "test";
+ try {
+ admin.transactions()
+ .getPendingAckInternalStatsAsync(topic, subName, true).get();
+ } catch (ExecutionException ex) {
+ assertTrue(ex.getCause() instanceof PulsarAdminException.NotFoundException);
+ PulsarAdminException.NotFoundException cause = (PulsarAdminException.NotFoundException)ex.getCause();
+ assertEquals(cause.getMessage(), "Topic not found");
+ }
+ try {
+ pulsar.getBrokerService().getTopic(topic, false);
+ admin.transactions()
+ .getPendingAckInternalStatsAsync(topic, subName, true).get();
Review comment:
yes, fixed.
--
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] Technoboy- commented on a change in pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r837406875
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TransactionsBase.java
##########
@@ -519,57 +519,52 @@ protected void internalGetPendingAckInternalStats(AsyncResponse asyncResponse, b
topicFuture.whenComplete((optionalTopic, e) -> {
if (e != null) {
- asyncResponse.resume(new RestException(e));
+ resumeAsyncResponseExceptionally(asyncResponse, e);
return;
}
if (!optionalTopic.isPresent()) {
- asyncResponse.resume(new RestException(TEMPORARY_REDIRECT,
- "Topic is not owned by this broker!"));
+ asyncResponse.resume(new RestException(NOT_FOUND, "Topic not found"));
return;
}
Topic topicObject = optionalTopic.get();
- if (topicObject instanceof PersistentTopic) {
- try {
- ManagedLedger managedLedger =
- ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
- TransactionPendingAckInternalStats stats =
- new TransactionPendingAckInternalStats();
- TransactionLogStats pendingAckLogStats = new TransactionLogStats();
- pendingAckLogStats.managedLedgerName = managedLedger.getName();
- pendingAckLogStats.managedLedgerInternalStats =
- managedLedger.getManagedLedgerInternalStats(metadata).get();
- stats.pendingAckLogStats = pendingAckLogStats;
- asyncResponse.resume(stats);
- } catch (Exception exception) {
- if (exception instanceof ExecutionException) {
- if (exception.getCause() instanceof ServiceUnitNotReadyException) {
- asyncResponse.resume(new RestException(SERVICE_UNAVAILABLE,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof NotAllowedException) {
- asyncResponse.resume(new RestException(METHOD_NOT_ALLOWED,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof SubscriptionNotFoundException) {
- asyncResponse.resume(new RestException(NOT_FOUND, exception.getCause()));
- return;
- }
+ try {
+ ManagedLedger managedLedger =
+ ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
Review comment:
Make `internalGetPendingAckInternalStats` fully async.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TransactionsBase.java
##########
@@ -519,57 +519,52 @@ protected void internalGetPendingAckInternalStats(AsyncResponse asyncResponse, b
topicFuture.whenComplete((optionalTopic, e) -> {
if (e != null) {
- asyncResponse.resume(new RestException(e));
+ resumeAsyncResponseExceptionally(asyncResponse, e);
return;
}
if (!optionalTopic.isPresent()) {
- asyncResponse.resume(new RestException(TEMPORARY_REDIRECT,
- "Topic is not owned by this broker!"));
+ asyncResponse.resume(new RestException(NOT_FOUND, "Topic not found"));
return;
}
Topic topicObject = optionalTopic.get();
- if (topicObject instanceof PersistentTopic) {
- try {
- ManagedLedger managedLedger =
- ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
- TransactionPendingAckInternalStats stats =
- new TransactionPendingAckInternalStats();
- TransactionLogStats pendingAckLogStats = new TransactionLogStats();
- pendingAckLogStats.managedLedgerName = managedLedger.getName();
- pendingAckLogStats.managedLedgerInternalStats =
- managedLedger.getManagedLedgerInternalStats(metadata).get();
- stats.pendingAckLogStats = pendingAckLogStats;
- asyncResponse.resume(stats);
- } catch (Exception exception) {
- if (exception instanceof ExecutionException) {
- if (exception.getCause() instanceof ServiceUnitNotReadyException) {
- asyncResponse.resume(new RestException(SERVICE_UNAVAILABLE,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof NotAllowedException) {
- asyncResponse.resume(new RestException(METHOD_NOT_ALLOWED,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof SubscriptionNotFoundException) {
- asyncResponse.resume(new RestException(NOT_FOUND, exception.getCause()));
- return;
- }
+ try {
+ ManagedLedger managedLedger =
+ ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
Review comment:
@eolivelli
--
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] Technoboy- closed pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876
--
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] Technoboy- commented on a change in pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r837288049
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TransactionsBase.java
##########
@@ -519,57 +519,52 @@ protected void internalGetPendingAckInternalStats(AsyncResponse asyncResponse, b
topicFuture.whenComplete((optionalTopic, e) -> {
if (e != null) {
- asyncResponse.resume(new RestException(e));
+ resumeAsyncResponseExceptionally(asyncResponse, e);
return;
}
if (!optionalTopic.isPresent()) {
- asyncResponse.resume(new RestException(TEMPORARY_REDIRECT,
- "Topic is not owned by this broker!"));
+ asyncResponse.resume(new RestException(NOT_FOUND, "Topic not found"));
return;
}
Topic topicObject = optionalTopic.get();
- if (topicObject instanceof PersistentTopic) {
- try {
- ManagedLedger managedLedger =
- ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
- TransactionPendingAckInternalStats stats =
- new TransactionPendingAckInternalStats();
- TransactionLogStats pendingAckLogStats = new TransactionLogStats();
- pendingAckLogStats.managedLedgerName = managedLedger.getName();
- pendingAckLogStats.managedLedgerInternalStats =
- managedLedger.getManagedLedgerInternalStats(metadata).get();
- stats.pendingAckLogStats = pendingAckLogStats;
- asyncResponse.resume(stats);
- } catch (Exception exception) {
- if (exception instanceof ExecutionException) {
- if (exception.getCause() instanceof ServiceUnitNotReadyException) {
- asyncResponse.resume(new RestException(SERVICE_UNAVAILABLE,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof NotAllowedException) {
- asyncResponse.resume(new RestException(METHOD_NOT_ALLOWED,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof SubscriptionNotFoundException) {
- asyncResponse.resume(new RestException(NOT_FOUND, exception.getCause()));
- return;
- }
+ try {
+ ManagedLedger managedLedger =
+ ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
Review comment:
Ok. I decided to change it to async, but a little from the original idea.
Anyway, we should avoid using it like this.
I will update later.
--
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 #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r838047146
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -222,7 +228,24 @@ public void getPendingAckInternalStats(@Suspended final AsyncResponse asyncRespo
@PathParam("topic") @Encoded String encodedTopic,
@PathParam("subName") String subName,
@QueryParam("metadata") @DefaultValue("false") boolean metadata) {
- internalGetPendingAckInternalStats(asyncResponse, authoritative,
- TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata);
+
+ internalGetPendingAckInternalStats(authoritative,
+ TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata)
Review comment:
I think we need to catch ``TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic)`` to avoid ``IllegalArgumentsException`` exception.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -222,7 +228,24 @@ public void getPendingAckInternalStats(@Suspended final AsyncResponse asyncRespo
@PathParam("topic") @Encoded String encodedTopic,
@PathParam("subName") String subName,
@QueryParam("metadata") @DefaultValue("false") boolean metadata) {
- internalGetPendingAckInternalStats(asyncResponse, authoritative,
- TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata);
+
+ internalGetPendingAckInternalStats(authoritative,
+ TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata)
+ .thenAccept(stats -> asyncResponse.resume(stats))
Review comment:
should we add success log here?
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java
##########
@@ -373,6 +375,23 @@ public void testGetPendingAckInternalStats() throws Exception {
TransactionImpl transaction = (TransactionImpl) getTransaction();
final String topic = "persistent://public/default/testGetPendingAckInternalStats";
final String subName = "test";
+ try {
+ admin.transactions()
+ .getPendingAckInternalStatsAsync(topic, subName, true).get();
Review comment:
Maybe we need to add ``Assert.fail()`` here.
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v3/AdminApiTransactionTest.java
##########
@@ -373,6 +375,23 @@ public void testGetPendingAckInternalStats() throws Exception {
TransactionImpl transaction = (TransactionImpl) getTransaction();
final String topic = "persistent://public/default/testGetPendingAckInternalStats";
final String subName = "test";
+ try {
+ admin.transactions()
+ .getPendingAckInternalStatsAsync(topic, subName, true).get();
+ } catch (ExecutionException ex) {
+ assertTrue(ex.getCause() instanceof PulsarAdminException.NotFoundException);
+ PulsarAdminException.NotFoundException cause = (PulsarAdminException.NotFoundException)ex.getCause();
+ assertEquals(cause.getMessage(), "Topic not found");
+ }
+ try {
+ pulsar.getBrokerService().getTopic(topic, false);
+ admin.transactions()
+ .getPendingAckInternalStatsAsync(topic, subName, true).get();
Review comment:
Maybe we need to add ``Assert.fail()`` here.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -222,7 +228,24 @@ public void getPendingAckInternalStats(@Suspended final AsyncResponse asyncRespo
@PathParam("topic") @Encoded String encodedTopic,
@PathParam("subName") String subName,
@QueryParam("metadata") @DefaultValue("false") boolean metadata) {
- internalGetPendingAckInternalStats(asyncResponse, authoritative,
- TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata);
+
+ internalGetPendingAckInternalStats(authoritative,
+ TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata)
+ .thenAccept(stats -> asyncResponse.resume(stats))
+ .exceptionally(ex -> {
+ Throwable cause = FutureUtil.unwrapCompletionException(ex);
Review comment:
should we add exception log here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a change in pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r838161669
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -222,7 +228,24 @@ public void getPendingAckInternalStats(@Suspended final AsyncResponse asyncRespo
@PathParam("topic") @Encoded String encodedTopic,
@PathParam("subName") String subName,
@QueryParam("metadata") @DefaultValue("false") boolean metadata) {
- internalGetPendingAckInternalStats(asyncResponse, authoritative,
- TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata);
+
+ internalGetPendingAckInternalStats(authoritative,
+ TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata)
Review comment:
yes, right. Thanks for this suggestion. The topic name isn't decoded...
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -222,7 +228,24 @@ public void getPendingAckInternalStats(@Suspended final AsyncResponse asyncRespo
@PathParam("topic") @Encoded String encodedTopic,
@PathParam("subName") String subName,
@QueryParam("metadata") @DefaultValue("false") boolean metadata) {
- internalGetPendingAckInternalStats(asyncResponse, authoritative,
- TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata);
+
+ internalGetPendingAckInternalStats(authoritative,
+ TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata)
Review comment:
fixed.
--
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] Technoboy- commented on a change in pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r838152610
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v3/Transactions.java
##########
@@ -222,7 +228,24 @@ public void getPendingAckInternalStats(@Suspended final AsyncResponse asyncRespo
@PathParam("topic") @Encoded String encodedTopic,
@PathParam("subName") String subName,
@QueryParam("metadata") @DefaultValue("false") boolean metadata) {
- internalGetPendingAckInternalStats(asyncResponse, authoritative,
- TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata);
+
+ internalGetPendingAckInternalStats(authoritative,
+ TopicName.get(TopicDomain.persistent.value(), tenant, namespace, encodedTopic), subName, metadata)
+ .thenAccept(stats -> asyncResponse.resume(stats))
+ .exceptionally(ex -> {
+ Throwable cause = FutureUtil.unwrapCompletionException(ex);
Review comment:
yes, 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
[GitHub] [pulsar] Shoothzj commented on pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#issuecomment-1080198229
@Technoboy- can we add some assertions on test?
--
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] eolivelli commented on a change in pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876#discussion_r837280589
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TransactionsBase.java
##########
@@ -519,57 +519,52 @@ protected void internalGetPendingAckInternalStats(AsyncResponse asyncResponse, b
topicFuture.whenComplete((optionalTopic, e) -> {
if (e != null) {
- asyncResponse.resume(new RestException(e));
+ resumeAsyncResponseExceptionally(asyncResponse, e);
return;
}
if (!optionalTopic.isPresent()) {
- asyncResponse.resume(new RestException(TEMPORARY_REDIRECT,
- "Topic is not owned by this broker!"));
+ asyncResponse.resume(new RestException(NOT_FOUND, "Topic not found"));
return;
}
Topic topicObject = optionalTopic.get();
- if (topicObject instanceof PersistentTopic) {
- try {
- ManagedLedger managedLedger =
- ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
- TransactionPendingAckInternalStats stats =
- new TransactionPendingAckInternalStats();
- TransactionLogStats pendingAckLogStats = new TransactionLogStats();
- pendingAckLogStats.managedLedgerName = managedLedger.getName();
- pendingAckLogStats.managedLedgerInternalStats =
- managedLedger.getManagedLedgerInternalStats(metadata).get();
- stats.pendingAckLogStats = pendingAckLogStats;
- asyncResponse.resume(stats);
- } catch (Exception exception) {
- if (exception instanceof ExecutionException) {
- if (exception.getCause() instanceof ServiceUnitNotReadyException) {
- asyncResponse.resume(new RestException(SERVICE_UNAVAILABLE,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof NotAllowedException) {
- asyncResponse.resume(new RestException(METHOD_NOT_ALLOWED,
- exception.getCause()));
- return;
- } else if (exception.getCause() instanceof SubscriptionNotFoundException) {
- asyncResponse.resume(new RestException(NOT_FOUND, exception.getCause()));
- return;
- }
+ try {
+ ManagedLedger managedLedger =
+ ((PersistentTopic) topicObject).getPendingAckManagedLedger(subName).get();
Review comment:
is this a async call ? in this case we should use "thenCompose" and concatenate the async calls
@mattisonchao FYI
--
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] Technoboy- closed pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876
--
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] eolivelli merged pull request #14876: [fix][broker] Fix getPendingAckInternalStats redirect issue.
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #14876:
URL: https://github.com/apache/pulsar/pull/14876
--
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