abhishekrb19 commented on code in PR #16127:
URL: https://github.com/apache/druid/pull/16127#discussion_r1525604477
##########
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java:
##########
@@ -278,13 +284,14 @@ public Response markSegmentsAsUnused(
);
return numUpdatedSegments;
};
- return performSegmentUpdate(dataSourceName, payload, operation);
+ return performSegmentUpdate(dataSourceName, payload, operation, true);
}
private Response performSegmentUpdate(
String dataSourceName,
MarkDataSourceSegmentsPayload payload,
- SegmentUpdateOperation operation
+ SegmentUpdateOperation operation,
+ boolean validateDatasourceIsQueryable
Review Comment:
hmm, this parameter name is slightly confusing. The short circuiting logic
below is an implementation detail for an optimization, so I don't think the
callers need to be say whether to skip the validation checks on datasource or
not.
How about calling it `boolean isMarkUsed` or something similar?
##########
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java:
##########
@@ -295,9 +302,11 @@ private Response performSegmentUpdate(
.build();
}
- final ImmutableDruidDataSource dataSource = getDataSource(dataSourceName);
- if (dataSource == null) {
- return logAndCreateDataSourceNotFoundResponse(dataSourceName);
+ if (validateDatasourceIsQueryable) {
Review Comment:
Perhaps include a comment to the effect of:
```suggestion
// We can skip the markUnused operation when there are no used segments for
this datasource.
// However, for the markUsed operation, we should still continue as we want
to mark a portion of the unused segments as used.
if (dataSource == null && !isMarkUsed) {
```
##########
server/src/test/java/org/apache/druid/server/http/DataSourcesResourceTest.java:
##########
@@ -817,23 +805,6 @@ public void
testMarkAsUsedNonOvershadowedSegmentsIntervalException()
EasyMock.verify(segmentsMetadataManager, inventoryView, server);
}
- @Test
- public void testMarkAsUsedNonOvershadowedSegmentsNoDataSource()
Review Comment:
Can we still keep this test and change the verification instead? i.e.,
instead of a 204 response code, I think the change should return a 200 with
`"numChangedSegments: 0` in the response.
--
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]
