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/13 04:24:32 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
mattisonchao opened a new pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259
### Motivation
Currently, some RestAPI will print an error log when the server returns the redirect code(307). which can let the user a lit bit confusing.
### Modifications
- Check if "RestException" is a redirect code.
- Clean up some unused methods.
- **Add some missing ``exceptionally``handlers that will make the request timeout.**
> like code lines 3048 - 3427
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `no-need-doc`
### Q&A
Q: Why remove code as follow:
```java
if (cause instanceof WebApplicationException
&& ((WebApplicationException) cause).getResponse().getStatus()
== Status.TEMPORARY_REDIRECT.getStatusCode()) {
if (log.isDebugEnabled()) {
log.debug("[{}] Failed to get subscriptions for non-partitioned topic {},"
+ " redirecting to other brokers.", clientAppId(), topicName, cause);
}
}
```
A: because some methods like ``validateTopicOwnershipAsync `` already have logged.
https://github.com/apache/pulsar/blob/ed563447f5fb8b34917ae343aebfc6a704515ed9/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java#L662-L674
### Additional
We have some code like this:
Multiple exception handlers will cause some code complexity, I think we can merge those handlers in another PR.
--
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 #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805456393
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
I think we need to use `getCause` here: `ex.getCause`.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
+ clientAppId(), topicName, ex);
Review comment:
Seems that we need to use `ex.getCause` 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] Demogorgon314 commented on a change in pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805489399
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3552,12 +3597,14 @@ protected void internalExpireMessagesByPosition(AsyncResponse asyncResponse, Str
}
});
}).exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to expire messages up to {} on subscription {} to position {}",
- clientAppId(), topicName, subName, messageId, cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
- return null;
- });
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to expire messages up to {} on subscription {} to position {}",
+ clientAppId(), topicName, subName, messageId, ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
```suggestion
resumeAsyncResponseExceptionally(asyncResponse, ex.getCause());
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3689,17 +3737,21 @@ protected void internalTriggerCompaction(AsyncResponse asyncResponse, boolean au
internalTriggerCompactionNonPartitionedTopic(asyncResponse, authoritative);
}
}).exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to trigger compaction on topic {}", clientAppId(), topicName, cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to trigger compaction on topic {}", clientAppId(), topicName, ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
```suggestion
resumeAsyncResponseExceptionally(asyncResponse, ex.getCause());
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
+ clientAppId(), topicName, ex);
}
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Need change to `ex.getCause()` too.
--
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 #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#issuecomment-1038863090
/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] RobertIndie commented on a change in pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805456393
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
I think we need to use `getCause` here: `ex.getCause`.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
+ clientAppId(), topicName, ex);
Review comment:
Seems that we need to use `ex.getCause` here.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Seems that we need to use `getCause` 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] mattisonchao commented on a change in pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805537951
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
+ clientAppId(), topicName, ex);
}
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @Demogorgon314
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
+ clientAppId(), topicName, ex);
}
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
return null;
});
}
}).exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to validate the global namespace ownership while unloading topic {}",
- clientAppId(), topicName, cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to validate the global namespace ownership while unloading topic {}",
+ clientAppId(), topicName, ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @Demogorgon314
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
+ clientAppId(), topicName, ex);
}
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
return null;
});
}
}).exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to validate the global namespace ownership while unloading topic {}",
- clientAppId(), topicName, cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to validate the global namespace ownership while unloading topic {}",
+ clientAppId(), topicName, ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @hangc0276
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
--
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] hangc0276 commented on a change in pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805495292
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -993,10 +994,12 @@ private void internalUnloadTransactionCoordinatorAsync(AsyncResponse asyncRespon
asyncResponse.resume(Response.noContent().build());
}))
.exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to unload tc {},{}", clientAppId(),
- topicName.getPartitionIndex(), cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to unload tc {},{}", clientAppId(),
+ topicName.getPartitionIndex(), ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
the same above
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
+ clientAppId(), topicName, ex);
}
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
return null;
});
}
}).exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to validate the global namespace ownership while unloading topic {}",
- clientAppId(), topicName, cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to validate the global namespace ownership while unloading topic {}",
+ clientAppId(), topicName, ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
ex.getCause()?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -1753,19 +1760,12 @@ protected void internalSkipAllMessages(AsyncResponse asyncResponse, String subNa
}
})
.exceptionally(ex -> {
- Throwable cause = FutureUtil.unwrapCompletionException(ex);
- if (cause instanceof WebApplicationException) {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Failed to skip all messages for subscription on topic {},"
- + " redirecting to other brokers.",
- clientAppId(), topicName, cause);
- }
- resumeAsyncResponseExceptionally(asyncResponse, cause);
- } else {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
log.error("[{}] Failed to skip all messages for subscription {} on topic {}",
- clientAppId(), subName, topicName, cause);
+ clientAppId(), subName, topicName, ex);
}
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
`FutureUtil.unwrapCompletionException(ex)` instead of `ex`?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -851,4 +851,17 @@ protected void validatePersistencePolicies(PersistencePolicies persistence) {
persistence.getBookkeeperAckQuorum()));
}
+
+ /**
+ * Check current exception whether is redirect exception.
+ *
+ * @param ex The throwable.
+ * @return Whether is redirect exception
+ */
+ protected static boolean isRedirectException(Throwable ex) {
+ Throwable realCause = FutureUtil.unwrapCompletionException(ex);
+ return realCause instanceof WebApplicationException
Review comment:
Do we need to add debug log here?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
Review comment:
Does it ok to change the original logic 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] hangc0276 commented on pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#issuecomment-1038626658
@mattisonchao Thanks for your contribution, please help resolve the conflicts, thanks a lot.
--
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 #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805457246
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Seems that we need to use `getCause` 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] mattisonchao commented on a change in pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805538946
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -753,22 +751,23 @@ protected void internalUnloadTopic(AsyncResponse asyncResponse, boolean authorit
} else {
internalUnloadNonPartitionedTopicAsync(asyncResponse, authoritative);
}
- }).exceptionally(t -> {
- log.error("[{}] Failed to get partitioned metadata while unloading topic {}",
- clientAppId(), topicName, t);
- if (t instanceof WebApplicationException) {
- asyncResponse.resume(t);
- } else {
- asyncResponse.resume(new RestException(t));
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
Review comment:
Looks like we don't change the original logic because ``resumeAsyncResponseExceptionally`` will do the same things.
I just let the redirect exception not log the error.
The detail:
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -1753,19 +1760,12 @@ protected void internalSkipAllMessages(AsyncResponse asyncResponse, String subNa
}
})
.exceptionally(ex -> {
- Throwable cause = FutureUtil.unwrapCompletionException(ex);
- if (cause instanceof WebApplicationException) {
- if (log.isDebugEnabled()) {
- log.debug("[{}] Failed to skip all messages for subscription on topic {},"
- + " redirecting to other brokers.",
- clientAppId(), topicName, cause);
- }
- resumeAsyncResponseExceptionally(asyncResponse, cause);
- } else {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
log.error("[{}] Failed to skip all messages for subscription {} on topic {}",
- clientAppId(), subName, topicName, cause);
+ clientAppId(), subName, topicName, ex);
}
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
``FutureUtil.unwrapCompletionException(ex)`` already exist in the ``resumeAsyncResponseExceptionally`` method.
--
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] codelipenghui commented on pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#issuecomment-1038584357
@mattisonchao Please rebase the master branch
--
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 #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#issuecomment-1038709051
@hangc0276 @Demogorgon314 @RobertIndie PTAL :)
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805544717
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -851,4 +851,17 @@ protected void validatePersistencePolicies(PersistencePolicies persistence) {
persistence.getBookkeeperAckQuorum()));
}
+
+ /**
+ * Check current exception whether is redirect exception.
+ *
+ * @param ex The throwable.
+ * @return Whether is redirect exception
+ */
+ protected static boolean isRedirectException(Throwable ex) {
+ Throwable realCause = FutureUtil.unwrapCompletionException(ex);
+ return realCause instanceof WebApplicationException
Review comment:
Because some method like ``validateTopicOwnership`` or ``validateNamespaceOwnership`` already has debug log. I'm not sure we should move that log here?
Details:
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java#L637-L689
--
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] codelipenghui merged pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259
--
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 #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805537592
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @RobertIndie
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
[The detail is
](https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774)
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @RobertIndie
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
[](https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774)
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @RobertIndie
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
--
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 #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805537862
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3552,12 +3597,14 @@ protected void internalExpireMessagesByPosition(AsyncResponse asyncResponse, Str
}
});
}).exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to expire messages up to {} on subscription {} to position {}",
- clientAppId(), topicName, subName, messageId, cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
- return null;
- });
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to expire messages up to {} on subscription {} to position {}",
+ clientAppId(), topicName, subName, messageId, ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @Demogorgon314
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -3689,17 +3737,21 @@ protected void internalTriggerCompaction(AsyncResponse asyncResponse, boolean au
internalTriggerCompactionNonPartitionedTopic(asyncResponse, authoritative);
}
}).exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to trigger compaction on topic {}", clientAppId(), topicName, cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to trigger compaction on topic {}", clientAppId(), topicName, ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @Demogorgon314
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
--
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 #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805547632
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -503,10 +503,13 @@ protected void internalCreateMissedPartitions(AsyncResponse asyncResponse) {
return null;
});
}
- }).exceptionally(e -> {
- log.error("[{}] Failed to create partitions for topic {}",
- clientAppId(), topicName);
- resumeAsyncResponseExceptionally(asyncResponse, e);
+ }).exceptionally(ex -> {
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to create partitions for topic {}",
+ clientAppId(), topicName);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Got it. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a change in pull request #14259: [Management] Fix print error log when server return redirect (http code 307).
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14259:
URL: https://github.com/apache/pulsar/pull/14259#discussion_r805538113
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -993,10 +994,12 @@ private void internalUnloadTransactionCoordinatorAsync(AsyncResponse asyncRespon
asyncResponse.resume(Response.noContent().build());
}))
.exceptionally(ex -> {
- Throwable cause = ex.getCause();
- log.error("[{}] Failed to unload tc {},{}", clientAppId(),
- topicName.getPartitionIndex(), cause);
- resumeAsyncResponseExceptionally(asyncResponse, cause);
+ // If the exception is not redirect exception we need to log it.
+ if (!isRedirectException(ex)) {
+ log.error("[{}] Failed to unload tc {},{}", clientAppId(),
+ topicName.getPartitionIndex(), ex);
+ }
+ resumeAsyncResponseExceptionally(asyncResponse, ex);
Review comment:
Hi @hangc0276
``resumeAsyncResponseExceptionally `` method has unwrapped the exception.
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L764-L774
--
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