Include the exception message in error response #14395
Conversation
| @@ -430,8 +430,8 @@ private SuccessResponse addSchema(Schema schema, boolean override, boolean force | |||
| throw new ControllerApplicationException(LOGGER, e.getMessage(), Response.Status.BAD_REQUEST, e); | |||
| } catch (Exception e) { | |||
| _controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_SCHEMA_UPLOAD_ERROR, 1L); | |||
| throw new ControllerApplicationException(LOGGER, String.format("Failed to add new schema %s.", schemaName), | |||
| Response.Status.INTERNAL_SERVER_ERROR, e); | |||
| throw new ControllerApplicationException(LOGGER, String.format("Failed to add new schema %s. Reason: %s", | |||
There was a problem hiding this comment.
Is there a format that we followed for such error logs?
I like this format as it will make easier to search/ grep.
There was a problem hiding this comment.
I vote against this messages created with String.format. They are difficult to read and may affect performance (in a negative way). In this case the performance is not problematic, but it is just a bad idea to try to do that everywhere
There was a problem hiding this comment.
As a developer who learnt the trade on C using printf and snprintf, this is the most natural templatization method for me :)
It is unfortunate that the Java implementation is terrible. A benchmark that shows the terrible performance of String.format. Source: https://stackoverflow.com/a/1281651
StringBuilder or + are possible candidates. I find StringBuilder more natural because + to me are for numbers and it confuses my brain for a new nanoseconds.
There was a problem hiding this comment.
Is there a format that we followed for such error logs?
I see this format being followed at other places where a custom message is followed by Reason: <exception message> hence extending the same here.
I vote against this messages created with String.format
Agree, in this PR I am not touching the existing formatted messages, just adding the reason part where its missing. Cleaning up all the String.format can be a separate PR altogether.
There was a problem hiding this comment.
opened an issue to discuss and finalize which option to use #14404
|
This may be a good temporal solution, but in general printing messages as they are is a bad practice. It can leak tons of information that shouldn't be leaked. The ideal solution would be to have our own exception (like PinotException) with a |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14395 +/- ##
============================================
+ Coverage 61.75% 63.77% +2.02%
- Complexity 207 1555 +1348
============================================
Files 2436 2660 +224
Lines 133233 145982 +12749
Branches 20636 22356 +1720
============================================
+ Hits 82274 93106 +10832
- Misses 44911 45994 +1083
- Partials 6048 6882 +834
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -369,8 +369,8 @@ private void persistInstancePartitionsHelper(InstancePartitions instancePartitio | |||
| InstancePartitionsUtils.persistInstancePartitions(_pinotHelixResourceManager.getPropertyStore(), | |||
| instancePartitions); | |||
| } catch (Exception e) { | |||
| throw new ControllerApplicationException(LOGGER, "Caught Exception while persisting the instance partitions", | |||
| Response.Status.INTERNAL_SERVER_ERROR, e); | |||
There was a problem hiding this comment.
nit: Why Message here vs Reason ?
There was a problem hiding this comment.
Ah good catch, will fix it!
| @@ -430,8 +430,8 @@ private SuccessResponse addSchema(Schema schema, boolean override, boolean force | |||
| throw new ControllerApplicationException(LOGGER, e.getMessage(), Response.Status.BAD_REQUEST, e); | |||
| } catch (Exception e) { | |||
| _controllerMetrics.addMeteredGlobalValue(ControllerMeter.CONTROLLER_SCHEMA_UPLOAD_ERROR, 1L); | |||
| throw new ControllerApplicationException(LOGGER, String.format("Failed to add new schema %s.", schemaName), | |||
| Response.Status.INTERNAL_SERVER_ERROR, e); | |||
| throw new ControllerApplicationException(LOGGER, String.format("Failed to add new schema %s. Reason: %s", | |||
There was a problem hiding this comment.
As a developer who learnt the trade on C using printf and snprintf, this is the most natural templatization method for me :)
It is unfortunate that the Java implementation is terrible. A benchmark that shows the terrible performance of String.format. Source: https://stackoverflow.com/a/1281651
StringBuilder or + are possible candidates. I find StringBuilder more natural because + to me are for numbers and it confuses my brain for a new nanoseconds.
Add the exception message to the rest endpoint error messages wherever missing so that user knows the reason of request failure without checking the logs