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/03/25 03:17:30 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #14856: Fix pulsar-admin not prompting message when there is a 500 error.
Technoboy- opened a new pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856
### Motivation
When pulsar-admin occurs 500 error, it doesn't return the root cause message to the user, only prompt `Internal Server Error`.
The root cause is that: the broker side return 'TEXT_PLAIN' type message,
https://github.com/apache/pulsar/blob/9644448fa2d85701e47fb8b6bf90ac149f70cf56/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestException.java#L77-L88
But client side tries to parse it as json type (line-222):
https://github.com/apache/pulsar/blob/9644448fa2d85701e47fb8b6bf90ac149f70cf56/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java#L219-L224
https://github.com/apache/pulsar/blob/9644448fa2d85701e47fb8b6bf90ac149f70cf56/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java#L272-L288
So if broker return 500, `getReasonFromServer` will throw exception 3 times and then use the original 500 message to the user.
There are two ways to solve the issue.
- Keep the server-side return TEXT_PLAIN, and change the client-slide not to parse it from JSON.
- Change the server-side return JSON, and keep the client-side keep the same.
In this pr, I use the second way to keep the client compatibility.
### 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- commented on a change in pull request #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856#discussion_r838146515
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestException.java
##########
@@ -80,9 +68,9 @@ private static Response getResponse(Throwable t) {
return e.getResponse();
} else {
return Response
- .status(Status.INTERNAL_SERVER_ERROR)
- .entity(getExceptionData(t))
- .type(MediaType.TEXT_PLAIN)
+ .status(Status.INTERNAL_SERVER_ERROR.getStatusCode(), t.getMessage())
+ .entity(new ErrorData(t.getMessage()))
Review comment:
ah, ok. yes, I removed that.
--
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 #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856#discussion_r838142591
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestException.java
##########
@@ -80,9 +68,9 @@ private static Response getResponse(Throwable t) {
return e.getResponse();
} else {
return Response
- .status(Status.INTERNAL_SERVER_ERROR)
- .entity(getExceptionData(t))
- .type(MediaType.TEXT_PLAIN)
+ .status(Status.INTERNAL_SERVER_ERROR.getStatusCode(), t.getMessage())
+ .entity(new ErrorData(t.getMessage()))
Review comment:
I notic that the old behavior with stack trace info.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestException.java
##########
@@ -80,9 +68,9 @@ private static Response getResponse(Throwable t) {
return e.getResponse();
} else {
return Response
- .status(Status.INTERNAL_SERVER_ERROR)
- .entity(getExceptionData(t))
- .type(MediaType.TEXT_PLAIN)
+ .status(Status.INTERNAL_SERVER_ERROR.getStatusCode(), t.getMessage())
+ .entity(new ErrorData(t.getMessage()))
Review comment:
I notice that the old behavior with stack trace info.
--
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 #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856#issuecomment-1085812883
> @Technoboy- Great improvement, but I'm just wondering how this will work when a newer client is used with an older broker version?
The client side has nothing changed. Newer clients with old brokers will keep the original behavior which means they are still getting `Internal Server Error.` when occurring 500 error.
--
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 #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856#discussion_r838131139
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestException.java
##########
@@ -80,9 +68,9 @@ private static Response getResponse(Throwable t) {
return e.getResponse();
} else {
return Response
- .status(Status.INTERNAL_SERVER_ERROR)
- .entity(getExceptionData(t))
- .type(MediaType.TEXT_PLAIN)
+ .status(Status.INTERNAL_SERVER_ERROR.getStatusCode(), t.getMessage())
+ .entity(new ErrorData(t.getMessage()))
Review comment:
What do you mean stack trace info ?
--
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 #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856#discussion_r838089638
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/RestException.java
##########
@@ -80,9 +68,9 @@ private static Response getResponse(Throwable t) {
return e.getResponse();
} else {
return Response
- .status(Status.INTERNAL_SERVER_ERROR)
- .entity(getExceptionData(t))
- .type(MediaType.TEXT_PLAIN)
+ .status(Status.INTERNAL_SERVER_ERROR.getStatusCode(), t.getMessage())
+ .entity(new ErrorData(t.getMessage()))
Review comment:
Should we ignore the stack trace info?
--
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 #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856#issuecomment-1085816117
> I'm not sure if it's change can affect exist user who use HTTP REST API
I think it's nothing impact.
--
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 #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856
--
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 pull request #14856: [fix][pulsar-admin] Fix pulsar-admin not prompting message when there is a 500 error.
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14856:
URL: https://github.com/apache/pulsar/pull/14856#issuecomment-1085771570
@Technoboy- Great improvement, but I'm just wondering how this will work when a newer client is used with an older broker version?
--
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