kfaraz commented on code in PR #16141:
URL: https://github.com/apache/druid/pull/16141#discussion_r1531541234
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -312,53 +327,91 @@ public int markSegments(final Collection<SegmentId>
segmentIds, final boolean us
}
/**
- * Marks all used segments that are *fully contained by* a particular
interval as unused.
+ * Marks all used segments that are <b>fully contained by</b> 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 <b>fully contained by</b> a particular
interval filtered by an optional list of versions
+ * as unused.
+ *
+ * @return Number of segments updated.
+ */
+ public int markSegmentsUnused(final String dataSource, final Interval
interval, @Nullable final List<String> versions)
{
if (Intervals.isEternity(interval)) {
- return handle
- .createStatement(
- StringUtils.format(
- "UPDATE %s SET used=:used, used_status_last_updated =
:used_status_last_updated "
- + "WHERE dataSource = :dataSource AND used = true",
- dbTables.getSegmentsTable()
- )
+ final StringBuilder sb = new StringBuilder();
+ sb.append(
+ StringUtils.format(
+ "UPDATE %s SET used=:used, used_status_last_updated =
:used_status_last_updated "
+ + "WHERE dataSource = :dataSource AND used = true",
+ dbTables.getSegmentsTable()
)
+ );
+
+ final boolean hasVersions = !CollectionUtils.isNullOrEmpty(versions);
Review Comment:
This doesn't seem right. The behaviour for null should not be the same as
empty.
The calling code might be filtering out the given list of versions based on
some condition, resulting in an empty list of versions being passed down to
this method. In such a case, this method should ideally return no results
instead of all versions.
If this was already the case in the existing code, we should consider fixing
it or atleast clearly calling this out in the javadocs of the whole chain of
methods that lead from the rest API endpoint down to this method.
PS: Treating null and empty as the same probably makes sense at the API
level, since the API would be clearly documented and would give out clear
validation messages such as `Versions must be specified` or `Versions not
required when specifying segment IDs`.
--
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]
