9aman commented on code in PR #16848:
URL: https://github.com/apache/pinot/pull/16848#discussion_r2374221347
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java:
##########
@@ -425,37 +362,71 @@ public void
removeAgedDeletedSegments(LeadControllerManager leadControllerManage
if (leadControllerManager.isLeaderForTable(tableName)) {
URI tableNameURI = URIUtils.getUri(deletedDirURI.toString(),
URIUtils.encode(tableName));
// Get files that are aged
- final String[] targetFiles = pinotFS.listFiles(tableNameURI,
false);
- int numFilesDeleted = 0;
- URI targetURI = null;
- for (String targetFile : targetFiles) {
+ final List<FileMetadata> targetFiles =
pinotFS.listFilesWithMetadata(tableNameURI, false);
+
+ if (targetFiles.isEmpty()) {
+ LOGGER.info("Deleting empty deleted segments directory: {} for
table: {}", tableNameURI, tableName);
try {
- targetURI =
- URIUtils.getUri(tableNameURI.toString(),
URIUtils.encode(URIUtils.getLastPart(targetFile)));
- long deletionTimeMs =
getDeletionTimeMsFromFile(targetURI.toString(),
pinotFS.lastModified(targetURI));
- if (System.currentTimeMillis() >= deletionTimeMs) {
- if (!deleteWithTimeout(pinotFS, targetURI, true,
OBJECT_DELETION_TIMEOUT, TimeUnit.SECONDS)) {
- LOGGER.warn("Failed to remove resource: {}", targetURI);
- } else {
- numFilesDeleted++;
- if (numFilesDeleted >=
NUM_AGED_SEGMENTS_TO_DELETE_PER_ATTEMPT) {
- LOGGER.info("Reached threshold of max aged segments to
delete per attempt. Deleted: {} files "
- + "from directory: {}", numFilesDeleted,
tableNameDir);
- break;
- }
- }
+ if (!pinotFS.delete(tableNameURI, false)) {
+ LOGGER.info("Could not delete deleted segments directory: {}
for table: {}", tableNameURI, tableName);
+ } else {
+ LOGGER.info("Successfully deleted deleted segments
directory: {} for table: {}", tableNameURI,
+ tableName);
}
} catch (Exception e) {
- LOGGER.warn("Failed to delete uri: {} for table: {}",
targetURI, tableName, e);
+ LOGGER.error("Exception occurred while deleting deleted
segments directory: {} for table: {}",
+ tableNameURI, tableName, e);
}
+ continue;
}
+ int numFilesDeleted = 0;
+ URI targetURI = null;
+ List<URI> targetURIs = new ArrayList<>();
+ for (FileMetadata targetFile : targetFiles) {
+ // Some file system implementations also return the current
table directory
+ // we do not want to delete the table directory
+ if (targetFile.isDirectory()) {
+ continue;
+ }
- if (numFilesDeleted == targetFiles.length) {
- // Delete directory if it's empty
- if (!deleteWithTimeout(pinotFS, tableNameURI, false,
OBJECT_DELETION_TIMEOUT, TimeUnit.SECONDS)) {
- LOGGER.warn("Failed to remove the directory: {}",
tableNameDir);
+ targetURI =
+ URIUtils.getUri(tableNameURI.toString(),
+
URIUtils.encode(URIUtils.getLastPart(targetFile.getFilePath())));
+ long deletionTimeMs =
getDeletionTimeMsFromFile(targetURI.toString(),
targetFile.getLastModifiedTime());
Review Comment:
`pinotFS.lastModified(targetURI) ` returns the same to what we would get if
we fetch the metadata using the getMetadata(). This seems to be invalid.
Moreover, this is only used when we can't extract the time to delete from
the segment name. This Ideally should not happen as is the case with the tests
as well.
--
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]
