This is an automated email from the ASF dual-hosted git repository.
kfaraz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new cf9a3bdc143 Fix up error handling in unusedSegments API. (#16206)
cf9a3bdc143 is described below
commit cf9a3bdc1439bbdcc71a598f1c2979f5bfe0b4c0
Author: Abhishek Radhakrishnan <[email protected]>
AuthorDate: Wed Mar 27 00:01:21 2024 -0700
Fix up error handling in unusedSegments API. (#16206)
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.
---
.../apache/druid/java/util/common/Intervals.java | 2 +-
.../druid/server/http/DataSourcesResource.java | 6 +-
.../apache/druid/server/http/MetadataResource.java | 62 ++++---
.../http/security/DatasourceResourceFilter.java | 29 +--
.../druid/server/http/MetadataResourceTest.java | 203 +++++++++++++--------
5 files changed, 169 insertions(+), 133 deletions(-)
diff --git
a/processing/src/main/java/org/apache/druid/java/util/common/Intervals.java
b/processing/src/main/java/org/apache/druid/java/util/common/Intervals.java
index 4da87648f0e..4dfe09144e6 100644
--- a/processing/src/main/java/org/apache/druid/java/util/common/Intervals.java
+++ b/processing/src/main/java/org/apache/druid/java/util/common/Intervals.java
@@ -44,7 +44,7 @@ public final class Intervals
return new Interval(interval, ISOChronology.getInstanceUTC());
}
catch (IllegalArgumentException e) {
- throw InvalidInput.exception(e, "Bad Interval[%s]: [%s]", interval,
e.getMessage());
+ throw InvalidInput.exception(e, "Invalid interval[%s]: [%s]", interval,
e.getMessage());
}
}
diff --git
a/server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java
b/server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java
index 2f4334b36ac..1614cb69074 100644
--- a/server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java
+++ b/server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java
@@ -40,7 +40,6 @@ import org.apache.druid.client.ImmutableSegmentLoadInfo;
import org.apache.druid.client.SegmentLoadInfo;
import org.apache.druid.common.guava.FutureUtils;
import org.apache.druid.error.DruidException;
-import org.apache.druid.error.ErrorResponse;
import org.apache.druid.error.InvalidInput;
import org.apache.druid.guice.annotations.PublicApi;
import org.apache.druid.java.util.common.DateTimes;
@@ -312,10 +311,7 @@ public class DataSourcesResource
return Response.ok(ImmutableMap.of("numChangedSegments",
numChangedSegments)).build();
}
catch (DruidException e) {
- return Response
- .status(e.getStatusCode())
- .entity(new ErrorResponse(e))
- .build();
+ return ServletResourceUtils.buildErrorResponseFrom(e);
}
catch (Exception e) {
log.error(e, "Error occurred while updating segments for
datasource[%s]", dataSourceName);
diff --git
a/server/src/main/java/org/apache/druid/server/http/MetadataResource.java
b/server/src/main/java/org/apache/druid/server/http/MetadataResource.java
index c53a998e238..3e2b8d56406 100644
--- a/server/src/main/java/org/apache/druid/server/http/MetadataResource.java
+++ b/server/src/main/java/org/apache/druid/server/http/MetadataResource.java
@@ -20,12 +20,15 @@
package org.apache.druid.server.http;
import com.google.common.base.Function;
+import com.google.common.base.Throwables;
import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.inject.Inject;
import com.sun.jersey.spi.container.ResourceFilters;
import org.apache.druid.client.DataSourcesSnapshot;
import org.apache.druid.client.ImmutableDruidDataSource;
+import org.apache.druid.error.DruidException;
import org.apache.druid.error.InvalidInput;
import org.apache.druid.indexing.overlord.IndexerMetadataStorageCoordinator;
import org.apache.druid.indexing.overlord.Segments;
@@ -340,6 +343,7 @@ public class MetadataResource
@GET
@Path("/datasources/{dataSourceName}/unusedSegments")
@Produces(MediaType.APPLICATION_JSON)
+ @ResourceFilters(DatasourceResourceFilter.class)
public Response getUnusedSegmentsInDataSource(
@Context final HttpServletRequest req,
@PathParam("dataSourceName") final String dataSource,
@@ -349,37 +353,43 @@ public class MetadataResource
@QueryParam("sortOrder") @Nullable final String sortOrder
)
{
- if (dataSource == null || dataSource.isEmpty()) {
- throw InvalidInput.exception("dataSourceName must be non-empty");
- }
- if (limit != null && limit < 0) {
- throw InvalidInput.exception("Invalid limit[%s] specified. Limit must be
> 0", limit);
- }
-
- if (lastSegmentId != null && SegmentId.tryParse(dataSource, lastSegmentId)
== null) {
- throw InvalidInput.exception("Invalid lastSegmentId[%s] specified.",
lastSegmentId);
- }
+ try {
+ if (dataSource == null || dataSource.isEmpty()) {
+ throw InvalidInput.exception("dataSourceName must be non-empty.");
+ }
- SortOrder theSortOrder = sortOrder == null ? null :
SortOrder.fromValue(sortOrder);
+ if (limit != null && limit < 0) {
+ throw InvalidInput.exception("Invalid limit[%s] specified. Limit must
be > 0.", limit);
+ }
- final Interval theInterval = interval != null ?
Intervals.of(interval.replace('_', '/')) : null;
- Iterable<DataSegmentPlus> unusedSegments =
segmentsMetadataManager.iterateAllUnusedSegmentsForDatasource(
- dataSource,
- theInterval,
- limit,
- lastSegmentId,
- theSortOrder
- );
+ if (lastSegmentId != null && SegmentId.tryParse(dataSource,
lastSegmentId) == null) {
+ throw InvalidInput.exception("Invalid lastSegmentId[%s] specified.",
lastSegmentId);
+ }
- final Function<DataSegmentPlus, Iterable<ResourceAction>> raGenerator =
segment -> Collections.singletonList(
-
AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(segment.getDataSegment().getDataSource()));
+ final SortOrder theSortOrder = sortOrder == null ? null :
SortOrder.fromValue(sortOrder);
- final Iterable<DataSegmentPlus> authorizedSegments =
- AuthorizationUtils.filterAuthorizedResources(req, unusedSegments,
raGenerator, authorizerMapper);
+ final Interval theInterval = interval != null ?
Intervals.of(interval.replace('_', '/')) : null;
+ final Iterable<DataSegmentPlus> unusedSegments =
segmentsMetadataManager.iterateAllUnusedSegmentsForDatasource(
+ dataSource,
+ theInterval,
+ limit,
+ lastSegmentId,
+ theSortOrder
+ );
- final List<DataSegmentPlus> retVal = new ArrayList<>();
- authorizedSegments.iterator().forEachRemaining(retVal::add);
- return Response.status(Response.Status.OK).entity(retVal).build();
+ final List<DataSegmentPlus> retVal = new ArrayList<>();
+ unusedSegments.iterator().forEachRemaining(retVal::add);
+ return Response.status(Response.Status.OK).entity(retVal).build();
+ }
+ catch (DruidException e) {
+ return ServletResourceUtils.buildErrorResponseFrom(e);
+ }
+ catch (Exception e) {
+ return Response
+ .serverError()
+ .entity(ImmutableMap.of("error", "Exception occurred.", "message",
Throwables.getRootCause(e).toString()))
+ .build();
+ }
}
@GET
diff --git
a/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java
b/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java
index e32446cfe5c..2e84e5bd1f3 100644
---
a/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java
+++
b/server/src/main/java/org/apache/druid/server/http/security/DatasourceResourceFilter.java
@@ -20,7 +20,6 @@
package org.apache.druid.server.http.security;
import com.google.common.base.Preconditions;
-import com.google.common.base.Predicate;
import com.google.common.collect.Iterables;
import com.google.inject.Inject;
import com.sun.jersey.spi.container.ContainerRequest;
@@ -33,16 +32,15 @@ import org.apache.druid.server.security.ResourceAction;
import org.apache.druid.server.security.ResourceType;
import javax.ws.rs.core.PathSegment;
+import java.util.List;
/**
- * Use this ResourceFilter when the datasource information is present after
"datasources" segment in the request Path
- * Here are some example paths where this filter is used -
- * - druid/coordinator/v1/datasources/{dataSourceName}/...
- * - druid/coordinator/v1/metadata/datasources/{dataSourceName}/...
- * - druid/v2/datasources/{dataSourceName}/...
+ * Use this resource filter for API endpoints that contain {@link
#DATASOURCES_PATH_SEGMENT} in their request path.
*/
public class DatasourceResourceFilter extends AbstractResourceFilter
{
+ private static final String DATASOURCES_PATH_SEGMENT = "datasources";
+
@Inject
public DatasourceResourceFilter(
AuthorizerMapper authorizerMapper
@@ -74,20 +72,11 @@ public class DatasourceResourceFilter extends
AbstractResourceFilter
private String getRequestDatasourceName(ContainerRequest request)
{
- final String dataSourceName = request.getPathSegments()
- .get(
- Iterables.indexOf(
- request.getPathSegments(),
- new Predicate<PathSegment>()
- {
- @Override
- public boolean
apply(PathSegment input)
- {
- return
"datasources".equals(input.getPath());
- }
- }
- ) + 1
- ).getPath();
+ final List<PathSegment> pathSegments = request.getPathSegments();
+ final String dataSourceName = pathSegments.get(
+ Iterables.indexOf(pathSegments, input ->
DATASOURCES_PATH_SEGMENT.equals(input.getPath())) + 1
+ ).getPath();
+
Preconditions.checkNotNull(dataSourceName);
return dataSourceName;
}
diff --git
a/server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java
b/server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java
index c76da048295..cc1e4b7bc58 100644
---
a/server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java
+++
b/server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java
@@ -25,10 +25,10 @@ import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import org.apache.druid.client.DataSourcesSnapshot;
import org.apache.druid.client.ImmutableDruidDataSource;
-import org.apache.druid.error.DruidExceptionMatcher;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.error.ErrorResponse;
import org.apache.druid.indexing.overlord.IndexerMetadataStorageCoordinator;
import org.apache.druid.java.util.common.DateTimes;
-import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.java.util.common.granularity.Granularities;
import org.apache.druid.java.util.common.guava.Comparators;
import org.apache.druid.metadata.SegmentsMetadataManager;
@@ -129,6 +129,15 @@ public class MetadataResourceTest
.when(storageCoordinator)
.retrieveSegmentForId(segments[5].getId().toString(), true);
+ Mockito.doAnswer(mockIterateAllUnusedSegmentsForDatasource())
+ .when(segmentsMetadataManager)
+ .iterateAllUnusedSegmentsForDatasource(
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any());
+
metadataResource = new MetadataResource(
segmentsMetadataManager,
storageCoordinator,
@@ -251,101 +260,124 @@ public class MetadataResourceTest
Assert.assertEquals(new SegmentStatusInCluster(realTimeSegments[1], false,
null, 40L, true), resultList.get(5));
}
+
@Test
public void testGetUnusedSegmentsInDataSource()
{
- Mockito.doAnswer(mockIterateAllUnusedSegmentsForDatasource())
- .when(segmentsMetadataManager)
- .iterateAllUnusedSegmentsForDatasource(
- ArgumentMatchers.any(),
- ArgumentMatchers.any(),
- ArgumentMatchers.any(),
- ArgumentMatchers.any(),
- ArgumentMatchers.any());
-
- // test with null datasource name - fails with expected bad datasource
name error
- DruidExceptionMatcher.invalidInput().expectMessageIs(
- "dataSourceName must be non-empty"
- ).assertThrowsAndMatches(
- () -> metadataResource.getUnusedSegmentsInDataSource(request, null,
null, null, null, null)
- );
-
- // test with empty datasource name - fails with expected bad datasource
name error
- DruidExceptionMatcher.invalidInput().expectMessageIs(
- "dataSourceName must be non-empty"
- ).assertThrowsAndMatches(
- () -> metadataResource.getUnusedSegmentsInDataSource(request, "",
null, null, null, null)
- );
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, null, null, null,
null);
+ final List<DataSegmentPlus> observedSegments =
extractResponseList(response);
+ Assert.assertEquals(segmentsPlus, observedSegments);
+ }
- // test invalid datasource - returns empty segments
- Response response = metadataResource.getUnusedSegmentsInDataSource(
- request,
- "invalid_datasource",
- null,
- null,
- null,
- null
- );
- List<DataSegmentPlus> resultList = extractResponseList(response);
- Assert.assertTrue(resultList.isEmpty());
-
- // test valid datasource with bad limit - fails with expected invalid
limit message
- DruidExceptionMatcher.invalidInput().expectMessageIs(
- StringUtils.format("Invalid limit[%s] specified. Limit must be > 0",
-1)
- ).assertThrowsAndMatches(
- () -> metadataResource.getUnusedSegmentsInDataSource(request,
DATASOURCE1, null, -1, null, null)
- );
+ @Test
+ public void testGetUnusedSegmentsInDataSourceWithIntervalFilter()
+ {
+ final String interval = SEGMENT_START_INTERVAL + "_P2D";
+ final int expectedLimit = NUM_PARTITIONS * 2;
+
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, interval, null,
null, null);
+ final List<DataSegmentPlus> observedSegments =
extractResponseList(response);
+ Assert.assertEquals(expectedLimit, observedSegments.size());
+
Assert.assertEquals(segmentsPlus.stream().limit(expectedLimit).collect(Collectors.toList()),
observedSegments);
+ }
- // test valid datasource with invalid lastSegmentId - fails with expected
invalid lastSegmentId message
- DruidExceptionMatcher.invalidInput().expectMessageIs(
- StringUtils.format("Invalid lastSegmentId[%s] specified.", "invalid")
- ).assertThrowsAndMatches(
- () -> metadataResource.getUnusedSegmentsInDataSource(request,
DATASOURCE1, null, null, "invalid", null)
- );
+ @Test
+ public void testGetUnusedSegmentsInDataSourceWithLimitFilter()
+ {
+ final int limit = 3;
+ final String interval = SEGMENT_START_INTERVAL + "_P2D";
+
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, interval, limit,
null, null);
+ final List<DataSegmentPlus> observedSegments =
extractResponseList(response);
+ Assert.assertEquals(limit, observedSegments.size());
+
Assert.assertEquals(segmentsPlus.stream().limit(limit).collect(Collectors.toList()),
observedSegments);
+ }
- // test valid datasource - returns all unused segments for that datasource
- response = metadataResource.getUnusedSegmentsInDataSource(request,
DATASOURCE1, null, null, null, null);
+ @Test
+ public void
testGetUnusedSegmentsInDataSourceWithLimitAndLastSegmentIdFilter()
+ {
+ final int limit = 3;
+ final String interval = SEGMENT_START_INTERVAL + "_P2D";
+ final String lastSegmentId = segments[limit - 1].getId().toString();
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, interval, limit,
lastSegmentId, null);
+ final List<DataSegmentPlus> observedSegments =
extractResponseList(response);
+ Assert.assertEquals(Collections.singletonList(segmentsPlus.get(limit)),
observedSegments);
+ }
- resultList = extractResponseList(response);
- Assert.assertEquals(segmentsPlus, resultList);
+ @Test
+ public void testGetUnusedSegmentsInDataSourceWithNonExistentDataSource()
+ {
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, "non-existent", null, null,
null, null);
+ final List<DataSegmentPlus> observedSegments =
extractResponseList(response);
+ Assert.assertTrue(observedSegments.isEmpty());
+ }
- // test valid datasource with interval filter - returns all unused
segments for that datasource within interval
- int numDays = 2;
- String interval = SEGMENT_START_INTERVAL + "_P" + numDays + "D";
- response = metadataResource.getUnusedSegmentsInDataSource(request,
DATASOURCE1, interval, null, null, null);
+ @Test
+ public void testGetUnusedSegmentsWithNoDataSource()
+ {
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, null, null, null, null, null);
+ Assert.assertEquals(400, response.getStatus());
+ Assert.assertEquals("dataSourceName must be non-empty.",
getExceptionMessage(response));
+ }
- resultList = extractResponseList(response);
- Assert.assertEquals(NUM_PARTITIONS * numDays, resultList.size());
- Assert.assertEquals(
- Arrays.asList(segmentsPlus.get(0), segmentsPlus.get(1),
segmentsPlus.get(2), segmentsPlus.get(3)),
- resultList
- );
+ @Test
+ public void testGetUnusedSegmentsWithEmptyDatasourceName()
+ {
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, "", null, null, null, null);
+ Assert.assertEquals(400, response.getStatus());
+ Assert.assertEquals("dataSourceName must be non-empty.",
getExceptionMessage(response));
+ }
- // test valid datasource with interval filter limit and last segment id -
returns unused segments for that
- // datasource within interval upto limit starting at last segment id
- int limit = 3;
- response = metadataResource.getUnusedSegmentsInDataSource(request,
DATASOURCE1, interval, limit, null, null);
+ @Test
+ public void testGetUnusedSegmentsWithInvalidLimit()
+ {
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, null, -1, null,
null);
+ Assert.assertEquals(400, response.getStatus());
+ Assert.assertEquals("Invalid limit[-1] specified. Limit must be > 0.",
getExceptionMessage(response));
+ }
- resultList = extractResponseList(response);
- Assert.assertEquals(limit, resultList.size());
- Assert.assertEquals(Arrays.asList(segmentsPlus.get(0),
segmentsPlus.get(1), segmentsPlus.get(2)), resultList);
+ @Test
+ public void testGetUnusedSegmentsWithInvalidLastSegmentId()
+ {
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, null, null,
"invalid", null);
+ Assert.assertEquals(400, response.getStatus());
+ Assert.assertEquals("Invalid lastSegmentId[invalid] specified.",
getExceptionMessage(response));
+ }
- // test valid datasource with interval filter limit and offset - returns
unused segments for that datasource within
- // interval upto limit starting at offset
- response = metadataResource.getUnusedSegmentsInDataSource(
- request,
- DATASOURCE1,
- interval,
- limit,
- segments[2].getId().toString(),
- null
+ @Test
+ public void testGetUnusedSegmentsWithInvalidInterval()
+ {
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, "2015/2014",
null, null, null);
+ Assert.assertEquals(400, response.getStatus());
+ Assert.assertEquals(
+ "Invalid interval[2015/2014]: [The end instant must be greater than
the start instant]",
+ getExceptionMessage(response)
);
+ }
- resultList = extractResponseList(response);
- Assert.assertEquals(Collections.singletonList(segmentsPlus.get(3)),
resultList);
+ @Test
+ public void testGetUnusedSegmentsWithInvalidSortOrder()
+ {
+ final Response response = metadataResource
+ .getUnusedSegmentsInDataSource(request, DATASOURCE1, null, null, null,
"Ascd");
+ Assert.assertEquals(400, response.getStatus());
+ Assert.assertEquals(
+ "Unexpected value[Ascd] for SortOrder. Possible values are: [ASC,
DESC]",
+ getExceptionMessage(response)
+ );
}
- Answer<Iterable<DataSegmentPlus>> mockIterateAllUnusedSegmentsForDatasource()
+ private Answer<Iterable<DataSegmentPlus>>
mockIterateAllUnusedSegmentsForDatasource()
{
return invocationOnMock -> {
String dataSourceName = invocationOnMock.getArgument(0);
@@ -375,6 +407,15 @@ public class MetadataResourceTest
};
}
+ private static String getExceptionMessage(final Response response)
+ {
+ if (response.getEntity() instanceof DruidException) {
+ return ((DruidException) response.getEntity()).getMessage();
+ } else {
+ return ((ErrorResponse)
response.getEntity()).getUnderlyingException().getMessage();
+ }
+ }
+
@Test
public void testGetDataSourceInformation()
{
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
