kfaraz commented on code in PR #16141:
URL: https://github.com/apache/druid/pull/16141#discussion_r1527413705
##########
docs/api-reference/data-management-api.md:
##########
@@ -206,20 +206,22 @@ Marks the state of a group of segments as unused, using
an array of segment IDs
Pass the array of segment IDs or interval as a JSON object in the request body.
For the interval, specify the start and end times as ISO 8601 strings to
identify segments inclusive of the start time and exclusive of the end time.
-Druid only updates the segments completely contained within the specified
interval; partially overlapping segments are not affected.
+Optionally, specify an array of segment versions with interval. Druid updates
only the segments completely contained
+within the specified interval that match the optional list of versions;
partially overlapping segments are not affected.
#### URL
<code class="postAPI">POST</code>
<code>/druid/coordinator/v1/datasources/:datasource/markUnused</code>
#### Request body
-The group of segments is sent as a JSON request payload that accepts one of
the following properties:
+The group of segments is sent as a JSON request payload that accepts the
following properties:
|Property|Description|Example|
|----------|-------------|---------|
|`interval`|ISO 8601 segments
interval.|`"2015-09-12T03:00:00.000Z/2015-09-12T05:00:00.000Z"`|
|`segmentIds`|Array of segment IDs.|`["segmentId1", "segmentId2"]`|
Review Comment:
Since the sentence before the table has been updated, we need to call out
that only one of `interval` or `segmentIds` is required.
Best way to do that would be to add a new column `Required`.
Also, I would advise using the word `List` instead of `Array` here.
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -834,6 +861,21 @@ private static int computeNumChangedSegments(List<String>
segmentIds, int[] segm
return numChangedSegments;
}
+ private static void appendConditionForVersions(
+ final StringBuilder sb,
+ final List<String> versions
+ )
+ {
+ if (CollectionUtils.isNullOrEmpty(versions)) {
Review Comment:
Nit: Better check this condition in the caller (even if it is being done in
two places) and call this method only when needed. Also, rename this method to
`getConditionForVersions` and return a `String` from this method rather than
passing the builder.
##########
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java:
##########
@@ -117,6 +118,9 @@ public class DataSourcesResource
private final DruidCoordinator coordinator;
private final AuditManager auditManager;
+ private final String invalidErrMsg = "Invalid request payload. Specify
either 'interval' or 'segmentIds', but not both."
Review Comment:
This constant is better placed inside the `MarkSegmentsPayload` class.
```suggestion
private static final String INVALID_PAYLOAD_ERROR_MESSAGE = "Invalid
request payload. Specify either 'interval' or 'segmentIds', but not both."
```
##########
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java:
##########
@@ -990,37 +993,59 @@ static boolean
isSegmentLoaded(Iterable<ImmutableSegmentLoadInfo> servedSegments
return false;
}
+ /**
+ * Either {@code interval} or {@code segmentIds} array must be specified,
but not both.
Review Comment:
Thanks for adding this!
##########
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java:
##########
@@ -990,37 +993,59 @@ static boolean
isSegmentLoaded(Iterable<ImmutableSegmentLoadInfo> servedSegments
return false;
}
+ /**
+ * Either {@code interval} or {@code segmentIds} array must be specified,
but not both.
+ * {@code versions} may be optionally specified only when {@code interval}
is provided.
+ */
@VisibleForTesting
- protected static class MarkDataSourceSegmentsPayload
+ static class MarkDataSourceSegmentsPayload
{
private final Interval interval;
private final Set<String> segmentIds;
+ private final List<String> versions;
@JsonCreator
public MarkDataSourceSegmentsPayload(
- @JsonProperty("interval") Interval interval,
- @JsonProperty("segmentIds") Set<String> segmentIds
+ @JsonProperty("interval") @Nullable Interval interval,
+ @JsonProperty("segmentIds") @Nullable Set<String> segmentIds,
+ @JsonProperty("versions") @Nullable List<String> versions
)
{
this.interval = interval;
this.segmentIds = segmentIds;
+ this.versions = versions;
}
+ @Nullable
@JsonProperty
public Interval getInterval()
{
return interval;
}
+ @Nullable
@JsonProperty
public Set<String> getSegmentIds()
{
return segmentIds;
}
- public boolean isValid()
+ @Nullable
+ @JsonProperty
+ public List<String> getVersions()
+ {
+ return versions;
+ }
+
+ private boolean isValid()
{
- return (interval == null ^ segmentIds == null) && (segmentIds == null ||
!segmentIds.isEmpty());
+ if (interval == null && CollectionUtils.isNullOrEmpty(segmentIds)) {
+ return false;
+ }
+ if (interval != null && segmentIds != null) {
+ return false;
+ }
+ return CollectionUtils.isNullOrEmpty(versions) || interval != null;
Review Comment:
Nit: A slight change to this might be more readable:
```suggestion
final boolean hasSegmentIds =
!CollectionUtils.isNullOrEmpty(segmentIds);
if (interval == null) {
return hasSegmentIds && CollectionUtils.isNullOrEmpty(versions);
} else {
return !hasSegmentIds;
}
```
##########
server/src/main/java/org/apache/druid/server/http/DataSourcesResource.java:
##########
@@ -990,37 +993,59 @@ static boolean
isSegmentLoaded(Iterable<ImmutableSegmentLoadInfo> servedSegments
return false;
}
+ /**
+ * Either {@code interval} or {@code segmentIds} array must be specified,
but not both.
+ * {@code versions} may be optionally specified only when {@code interval}
is provided.
+ */
@VisibleForTesting
- protected static class MarkDataSourceSegmentsPayload
+ static class MarkDataSourceSegmentsPayload
Review Comment:
Is it still serializable if it is package protected?
Nit: How about we rename this to `SegmentsToUpdateFilter` or just
`SegmentsToUpdate`?
The current name doesn't give any idea as to the content of the class and
how will it be a used. ("marking a segment" doesn't mean much unless we throw
"used/unused" in the mix).
##########
server/src/main/java/org/apache/druid/metadata/SegmentsMetadataManager.java:
##########
@@ -53,7 +53,20 @@ public interface SegmentsMetadataManager
*/
int markAsUsedAllNonOvershadowedSegmentsInDataSource(String dataSource);
- int markAsUsedNonOvershadowedSegmentsInInterval(String dataSource, Interval
interval);
+ /**
+ * Marks non-overshadowed unused segments for the given interval as used.
+ * @return Number of segments updated
+ */
+ default int markAsUsedNonOvershadowedSegmentsInInterval(String dataSource,
Interval interval)
Review Comment:
If it is used only in tests, we can get rid of it.
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -314,19 +327,34 @@ public int markSegments(final Collection<SegmentId>
segmentIds, final boolean us
/**
* Marks all used segments that are *fully contained by* a particular
interval as unused.
*
- * @return the number of segments actually modified.
+ * @return Number of segments updated.
*/
public int markSegmentsUnused(final String dataSource, final Interval
interval)
+ {
+ return markSegmentsUnused(dataSource, interval, null);
+ }
+
+ /**
+ * Marks all used segments that are *fully contained by* a particular
interval filtered by an optional list of versions
Review Comment:
Nit: I don't think the markdown highlighting style with asterisks would work
here. You can use html tags `<b></b>` or `<i></i>` instead.
--
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]
