Skip to content

Fix up error handling in unusedSegments API.#16206

Merged
kfaraz merged 4 commits intoapache:masterfrom
abhishekrb19:fix_unused_segments_error_handling
Mar 27, 2024
Merged

Fix up error handling in unusedSegments API.#16206
kfaraz merged 4 commits intoapache:masterfrom
abhishekrb19:fix_unused_segments_error_handling

Conversation

@abhishekrb19
Copy link
Contributor

@abhishekrb19 abhishekrb19 commented Mar 26, 2024 •
edited
Loading

Problem:

Before this patch, the /datasources/{dataSourceName}/unusedSegments API throws a 500 with a huge stack trace for any invalid parameters specified in the request. This is because DruidExceptions are thrown without mapping them to a HTTP response.

Click here to see the stacktrace with a 500 error code 
curl -X 'GET' -H 'Content-Type:application/json' http://localhost:8081/druid/coordinator/v1/metadata/datasources/foo_it/unusedSegments\?interval=2024-04-01T00%3A00%3A00.000Z/2024-03-01T00%3A00%3A00.000Z
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 500 org.apache.druid.error.DruidException: Bad Interval[2024-04-01T00:00:00.000Z/2024-03-01T00:00:00.000Z]: [The end instant must be greater than the start instant]</title>
</head>
<body><h2>HTTP ERROR 500 org.apache.druid.error.DruidException: Bad Interval[2024-04-01T00:00:00.000Z/2024-03-01T00:00:00.000Z]: [The end instant must be greater than the start instant]</h2>
<table>
<tr><th>URI:</th><td>/druid/coordinator/v1/metadata/datasources/foo_it/unusedSegments</td></tr>
<tr><th>STATUS:</th><td>500</td></tr>
<tr><th>MESSAGE:</th><td>org.apache.druid.error.DruidException: Bad Interval[2024-04-01T00:00:00.000Z/2024-03-01T00:00:00.000Z]: [The end instant must be greater than the start instant]</td></tr>
<tr><th>SERVLET:</th><td>default</td></tr>
<tr><th>CAUSED BY:</th><td>org.apache.druid.error.DruidException: Bad Interval[2024-04-01T00:00:00.000Z/2024-03-01T00:00:00.000Z]: [The end instant must be greater than the start instant]</td></tr>
<tr><th>CAUSED BY:</th><td>java.lang.IllegalArgumentException: The end instant must be greater than the start instant</td></tr>
</table>
<h3>Caused by:</h3><pre>org.apache.druid.error.DruidException: Bad Interval[2024-04-01T00:00:00.000Z/2024-03-01T00:00:00.000Z]: [The end instant must be greater than the start instant]
  at org.apache.druid.error.DruidException$DruidExceptionBuilder.build(DruidException.java:460)
  at org.apache.druid.error.BaseFailure.makeException(BaseFailure.java:57)
  at org.apache.druid.error.DruidException.fromFailure(DruidException.java:154)
  at org.apache.druid.error.InvalidInput.exception(InvalidInput.java:35)
  at org.apache.druid.java.util.common.Intervals.of(Intervals.java:47)
  at org.apache.druid.server.http.MetadataResource.getUnusedSegmentsInDataSource(MetadataResource.java:365)
  at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
  at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
  at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  at java.base/java.lang.reflect.Method.invoke(Method.java:568)

Changes:

  • Handle exceptions in the API and map them to a Response object with the appropriate error code.
  • Replace AuthorizationUtils.filterAuthorizedResources() with DatasourceResourceFilter. The endpoint is annotated consistent with other usages.
  • Update DatasourceResourceFilter to remove the lambda and update javadocs. The usages information is self-evident with an IDE.
  • Adjust the invalid interval exception message.
  • Break up the large unit test testGetUnusedSegmentsInDataSource() into smaller unit tests for each test case. Also, validate the error codes.

With the fix in this patch, a 400 error code is returned without the stacktrace:

curl -X 'GET' -H 'Content-Type:application/json' http://localhost:8081/druid/coordinator/v1/metadata/datasources/foo_it/unusedSegments\?interval=2024-04-01T00%3A00%3A00.000Z/2024-03-01T00%3A00%3A00.000Z
{"error":"druidException","errorCode":"invalidInput","persona":"USER","category":"INVALID_INPUT","errorMessage":"Invalid interval[2024-04-01T00:00:00.000Z/2024-03-01T00:00:00.000Z]: [The end instant must be greater than the start instant]","context":{}}

This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

@abhishekrb19
Fix up error handling for unusedSegments API.
0fcaded 
- Throwing DruidException directly without wrapping it up in a proper
  Response object causes the server to treat them as 500 with a huge
  junk stacktrace. This PR fixes that by handling DruidException and any
  other exception in this API.
- Also, clean up and break up the unit tests such that each test case
  is on its own. Validate the status codes in addition to the exception
  messages.
@abhishekrb19 abhishekrb19 requested review from kfaraz and zachjsh March 26, 2024 16:15
abhishekrb19
abhishekrb19 commented Mar 26, 2024
View reviewed changes
server/src/main/java/org/apache/druid/server/http/MetadataResource.java Outdated
return Response.status(Response.Status.OK).entity(retVal).build();
}
catch (DruidException e) {
return Response
Copy link
Contributor Author

@abhishekrb19 abhishekrb19 Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be nice if we can hook this into the web-server somehow so all APIs benefits. I haven't looked into how to do it, though.

Copy link
Contributor

@kfaraz kfaraz Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to write a ResourceFilter and tag all relevant API endpoint methods with @ResourceFilter(DruidExceptionMappingFilter.class).

Copy link
Contributor Author

@abhishekrb19 abhishekrb19 Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍 Can look into wiring that up for all the endpoints in a follow up.

kfaraz
kfaraz reviewed Mar 26, 2024
View reviewed changes
server/src/main/java/org/apache/druid/server/http/MetadataResource.java Outdated
return Response.status(Response.Status.OK).entity(retVal).build();
}
catch (DruidException e) {
return Response
Copy link
Contributor

@kfaraz kfaraz Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to write a ResourceFilter and tag all relevant API endpoint methods with @ResourceFilter(DruidExceptionMappingFilter.class).

abhishekrb19 and others added 2 commits March 26, 2024 11:21
@abhishekrb19 @kfaraz
Apply suggestions from code review
659ae21 
Co-authored-by: Kashif Faraz <kashif.faraz@gmail.com>
@abhishekrb19
Checkstyle and error message fix.
8d03234
@abhishekrb19 abhishekrb19 requested a review from kfaraz March 26, 2024 18:39
zachjsh
zachjsh reviewed Mar 26, 2024
View reviewed changes
server/src/main/java/org/apache/druid/server/http/MetadataResource.java
authorizedSegments.iterator().forEachRemaining(retVal::add);
return Response.status(Response.Status.OK).entity(retVal).build();
}
catch (DruidException e) {
Copy link
Contributor

@zachjsh zachjsh Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we not instead map InvalidInput exception to the appropriate response in some higher layer exception mapper so that all apis benefit, instead of doing this try catch here?

Copy link
Contributor Author

@abhishekrb19 abhishekrb19 Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree and had similar thoughts in this comment: #16206 (comment)

zachjsh
zachjsh approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@zachjsh zachjsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abhishekrb19 abhishekrb19 force-pushed the fix_unused_segments_error_handling branch from 4333e3c to a0f8b49 Compare March 26, 2024 20:12
kfaraz
kfaraz approved these changes Mar 27, 2024
View reviewed changes
server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java
) + 1
).getPath();
final List<PathSegment> pathSegments = request.getPathSegments();
final String dataSourceName = pathSegments.get(
Copy link
Contributor

@kfaraz kfaraz Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for simplifying this!

@kfaraz kfaraz merged commit cf9a3bd into apache:master Mar 27, 2024
@abhishekrb19 abhishekrb19 deleted the fix_unused_segments_error_handling branch March 27, 2024 17:44
@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zachjsh zachjsh zachjsh approved these changes

@kfaraz kfaraz kfaraz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

30.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@abhishekrb19 @zachjsh @kfaraz @adarshsanjeev