FrankChen021 commented on code in PR #19772:
URL: https://github.com/apache/druid/pull/19772#discussion_r3674249984
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -1706,6 +1775,26 @@ private Interval mapToInterval(ResultSet resultSet,
String dataSource)
}
}
+ /**
+ * Reads the fields {@code dataSource}, {@code start}, {@code end} and
+ * {@code totalCount} from the given result set.
+ */
+ @Nullable
+ private Pair<DatasourceInterval, Integer>
mapToUnusedSegmentInterval(ResultSet resultSet)
+ {
+ try {
+ final String dataSource = resultSet.getString("dataSource");
+ final Interval interval = mapToInterval(resultSet, "");
+ final int totalCount = resultSet.getInt("totalCount");
+
+ return Pair.of(new DatasourceInterval(dataSource, interval), totalCount);
Review Comment:
Thanks. I rechecked the current head across all 10 of 10 changed files, and
this remains present: `mapToInterval` can return null, but
`mapToUnusedSegmentInterval` still constructs `DatasourceInterval(dataSource,
null)`. The outer non-null filter therefore retains the row, and queue
rebuilding later dereferences the null interval. Please check `interval` before
constructing the pair.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/duty/UnusedSegmentsKiller.java:
##########
@@ -462,7 +490,7 @@ protected List<DataSegmentPlus>
fetchNextBatchOfUnusedSegments(TaskToolbox toolb
getDataSource(),
getInterval(),
getMaxUsedStatusLastUpdatedTime(),
- MAX_SEGMENTS_TO_KILL_IN_INTERVAL
+ MAX_SEGMENTS_TO_KILL_IN_BATCH
Review Comment:
Thanks—the override now forwards `nextBatchSize`, but the candidate limit is
still not enforced when some unused segments share a load spec.
`KillUnusedSegmentsTask` increments `numSegmentsKilled` by the number deleted
from deep storage, although `SegmentNukeAction` removes the full fetched batch
from metadata. For example, after a 1,001-row candidate fetches 1,000 rows
whose shared load specs prevent deep-store deletion, the next batch size
remains 1,000 and another 1,000 metadata rows can be removed. Please track
metadata rows processed or otherwise cap this override independently. I
rechecked all 10 of 10 changed files.
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataQuery.java:
##########
@@ -1116,6 +1118,70 @@ public List<Interval>
retrieveSomeUnusedSegmentIntervals(String dataSource, int
return
intervals.stream().filter(Objects::nonNull).collect(Collectors.toList());
}
+ /**
+ * Scans upto {@code maxSegmentsToScan} unused segments which are eligible
for
+ * kill and returns the unique datasource-interval for the segments scanned.
+ * <p>
+ * This method ensures that if there is any unused segment in any datasource
+ * which was updated earlier than {@code maxUpdatedTime}, then the returned
+ * map is not empty. However, it does NOT guarantee that:
+ * <ul>
+ * <li>the candidates in the returned map would be ordered by datasource or
interval</li>
+ * <li>the result would contain {@code limit} entries when there are more
distinct
+ * intervals with eligible unused segments in the metadata store.</li>
+ * </ul>
+ *
+ * @param maxUpdatedTime Unused segments are considered eligible for kill
+ * if they were last updated before this time.
+ * @param maxResultSize Maximum number of candidate intervals to return
+ * across all datasources.
+ * @param maxSegmentsToScan Maximum number of eligible unused segments to
scan
+ * in the metadata store.
+ * @return Map from {@link DatasourceInterval} to the number of unused
segments
+ * eligible for kill.
+ */
+ public Map<DatasourceInterval, Integer> retrieveSomeUnusedSegmentIntervals(
+ DateTime maxUpdatedTime,
+ int maxResultSize,
+ int maxSegmentsToScan
+ )
+ {
+ final String sql = StringUtils.format(
+ // Disable checkstyle to avoid argumentLineBreaking rule from getting
triggered
+ //CHECKSTYLE.OFF: Regexp
+ """
+ SELECT dataSource, start, %2$send%2$s, COUNT(*) AS totalCount
+ FROM (
+ SELECT dataSource, start, %2$send%2$s
+ FROM %1$s
+ WHERE used = false
+ AND (used_status_last_updated IS NULL OR
used_status_last_updated <= :maxUpdatedTime)
+ %3$s
+ ) AS unused
+ GROUP BY dataSource, %2$send%2$s, start
+ %4$s
+ """,
+ //CHECKSTYLE.ON: Regexp
+ dbTables.getSegmentsTable(),
+ connector.getQuoteString(),
+ connector.limitClause(maxSegmentsToScan),
Review Comment:
Thanks for the context. After rechecking all 10 of 10 changed files, I would
retain this finding. With no ordering, cursor, or rotation state, a stable
query plan can return the same physical prefix on each rebuild. The locks need
not be literally permanent: ongoing late-data activity can keep a prefix of
streaming intervals locked while every row outside the global cap, including
unrelated datasources, remains undiscovered. Since concurrent locking is
deferred, this PR still needs deterministic progress or fairness.
--
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]
