kfaraz commented on code in PR #16206:
URL: https://github.com/apache/druid/pull/16206#discussion_r1539842647
##########
server/src/main/java/org/apache/druid/server/http/MetadataResource.java:
##########
@@ -349,37 +353,52 @@ public Response getUnusedSegmentsInDataSource(
@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);
- }
+ try {
+ if (dataSource == null || dataSource.isEmpty()) {
+ throw InvalidInput.exception("dataSourceName must be non-empty.");
+ }
- if (lastSegmentId != null && SegmentId.tryParse(dataSource, lastSegmentId)
== null) {
- throw InvalidInput.exception("Invalid lastSegmentId[%s] specified.",
lastSegmentId);
- }
+ if (limit != null && limit < 0) {
+ throw InvalidInput.exception("Invalid limit[%s] specified. Limit must
be > 0.", limit);
+ }
- SortOrder theSortOrder = sortOrder == null ? null :
SortOrder.fromValue(sortOrder);
+ if (lastSegmentId != null && SegmentId.tryParse(dataSource,
lastSegmentId) == null) {
+ throw InvalidInput.exception("Invalid lastSegmentId[%s] specified.",
lastSegmentId);
+ }
- final Interval theInterval = interval != null ?
Intervals.of(interval.replace('_', '/')) : null;
- Iterable<DataSegmentPlus> unusedSegments =
segmentsMetadataManager.iterateAllUnusedSegmentsForDatasource(
- dataSource,
- theInterval,
- limit,
- lastSegmentId,
- theSortOrder
- );
+ final SortOrder theSortOrder = sortOrder == null ? null :
SortOrder.fromValue(sortOrder);
+
+ final Interval theInterval = interval != null ?
Intervals.of(interval.replace('_', '/')) : null;
+ final Iterable<DataSegmentPlus> unusedSegments =
segmentsMetadataManager.iterateAllUnusedSegmentsForDatasource(
+ dataSource,
+ theInterval,
+ limit,
+ lastSegmentId,
+ theSortOrder
+ );
- final Function<DataSegmentPlus, Iterable<ResourceAction>> raGenerator =
segment -> Collections.singletonList(
-
AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(segment.getDataSegment().getDataSource()));
+ final Function<DataSegmentPlus, Iterable<ResourceAction>> raGenerator =
segment -> Collections.singletonList(
+
AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(segment.getDataSegment().getDataSource()));
- final Iterable<DataSegmentPlus> authorizedSegments =
- AuthorizationUtils.filterAuthorizedResources(req, unusedSegments,
raGenerator, authorizerMapper);
+ final Iterable<DataSegmentPlus> authorizedSegments =
+ AuthorizationUtils.filterAuthorizedResources(req, unusedSegments,
raGenerator, authorizerMapper);
- 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<>();
+ authorizedSegments.iterator().forEachRemaining(retVal::add);
+ return Response.status(Response.Status.OK).entity(retVal).build();
+ }
+ catch (DruidException e) {
+ return Response
Review Comment:
You can also use
`ServletResourceUtils.buildErrorResponseFrom(druidException)`.
##########
server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java:
##########
@@ -129,6 +129,15 @@ public void setUp()
.when(storageCoordinator)
.retrieveSegmentForId(segments[5].getId().toString(), true);
+ Mockito.doAnswer(mockIterateAllUnusedSegmentsForDatasource())
+ .when(segmentsMetadataManager)
+ .iterateAllUnusedSegmentsForDatasource(
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any(),
+ ArgumentMatchers.any());
Review Comment:
```suggestion
ArgumentMatchers.any()
);
```
##########
server/src/test/java/org/apache/druid/server/http/MetadataResourceTest.java:
##########
@@ -251,101 +260,124 @@ public void testGetAllSegmentsIncludingRealtime()
Assert.assertEquals(new SegmentStatusInCluster(realTimeSegments[1], false,
null, 40L, true), resultList.get(5));
}
+
@Test
- public void testGetUnusedSegmentsInDataSource()
+ public void testGetUnusedSegmentsInDataSourceWithValidDataSource()
Review Comment:
this test could just be named `testGetUnusedSegmentsInDataSource` as it is a
positive test anyway.
##########
server/src/main/java/org/apache/druid/server/http/MetadataResource.java:
##########
@@ -349,37 +353,52 @@ public Response getUnusedSegmentsInDataSource(
@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);
- }
+ try {
+ if (dataSource == null || dataSource.isEmpty()) {
+ throw InvalidInput.exception("dataSourceName must be non-empty.");
+ }
- if (lastSegmentId != null && SegmentId.tryParse(dataSource, lastSegmentId)
== null) {
- throw InvalidInput.exception("Invalid lastSegmentId[%s] specified.",
lastSegmentId);
- }
+ if (limit != null && limit < 0) {
+ throw InvalidInput.exception("Invalid limit[%s] specified. Limit must
be > 0.", limit);
+ }
- SortOrder theSortOrder = sortOrder == null ? null :
SortOrder.fromValue(sortOrder);
+ if (lastSegmentId != null && SegmentId.tryParse(dataSource,
lastSegmentId) == null) {
+ throw InvalidInput.exception("Invalid lastSegmentId[%s] specified.",
lastSegmentId);
+ }
- final Interval theInterval = interval != null ?
Intervals.of(interval.replace('_', '/')) : null;
- Iterable<DataSegmentPlus> unusedSegments =
segmentsMetadataManager.iterateAllUnusedSegmentsForDatasource(
- dataSource,
- theInterval,
- limit,
- lastSegmentId,
- theSortOrder
- );
+ final SortOrder theSortOrder = sortOrder == null ? null :
SortOrder.fromValue(sortOrder);
+
+ final Interval theInterval = interval != null ?
Intervals.of(interval.replace('_', '/')) : null;
+ final Iterable<DataSegmentPlus> unusedSegments =
segmentsMetadataManager.iterateAllUnusedSegmentsForDatasource(
+ dataSource,
+ theInterval,
+ limit,
+ lastSegmentId,
+ theSortOrder
+ );
- final Function<DataSegmentPlus, Iterable<ResourceAction>> raGenerator =
segment -> Collections.singletonList(
-
AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(segment.getDataSegment().getDataSource()));
+ final Function<DataSegmentPlus, Iterable<ResourceAction>> raGenerator =
segment -> Collections.singletonList(
+
AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(segment.getDataSegment().getDataSource()));
- final Iterable<DataSegmentPlus> authorizedSegments =
- AuthorizationUtils.filterAuthorizedResources(req, unusedSegments,
raGenerator, authorizerMapper);
+ final Iterable<DataSegmentPlus> authorizedSegments =
+ AuthorizationUtils.filterAuthorizedResources(req, unusedSegments,
raGenerator, authorizerMapper);
- 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<>();
+ authorizedSegments.iterator().forEachRemaining(retVal::add);
+ return Response.status(Response.Status.OK).entity(retVal).build();
+ }
+ catch (DruidException e) {
+ return Response
Review Comment:
You would need to write a `ResourceFilter` and tag all relevant API endpoint
methods with `@ResourceFilter(DruidExceptionMappingFilter.class)`.
##########
processing/src/main/java/org/apache/druid/java/util/common/Intervals.java:
##########
@@ -44,7 +44,7 @@ public static Interval of(String interval)
return new Interval(interval, ISOChronology.getInstanceUTC());
}
catch (IllegalArgumentException e) {
- throw InvalidInput.exception(e, "Bad Interval[%s]: [%s]", interval,
e.getMessage());
+ throw InvalidInput.exception(e, "Bad interval[%s]: [%s]", interval,
e.getMessage());
Review Comment:
```suggestion
throw InvalidInput.exception(e, "Invalid interval[%s]: [%s]",
interval, e.getMessage());
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
