vigneshio commented on code in PR #4828:
URL: https://github.com/apache/polaris/pull/4828#discussion_r3442253254
##########
runtime/service/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java:
##########
@@ -80,37 +81,42 @@ private boolean cleanUpManifestFile(
return true;
}
- ManifestReader<DataFile> dataFiles = ManifestFiles.read(manifestFile,
fileIO, null);
- List<CompletableFuture<Void>> dataFileDeletes =
- StreamSupport.stream(
- Spliterators.spliteratorUnknownSize(dataFiles.iterator(),
Spliterator.IMMUTABLE),
- false)
- .map(file -> tryDelete(tableId, fileIO, manifestFile.path(),
file.location(), null, 1))
- .toList();
- LOGGER.debug(
- "Scheduled {} data files to be deleted from manifest {}",
- dataFileDeletes.size(),
- manifestFile.path());
- try {
- // wait for all data files to be deleted, then wait for the manifest
itself to be deleted
-
CompletableFuture.allOf(dataFileDeletes.toArray(CompletableFuture[]::new))
- .thenCompose(
- (v) -> {
- LOGGER
- .atInfo()
- .addKeyValue("manifestFile", manifestFile.path())
- .log("All data files in manifest deleted - deleting
manifest");
- return tryDelete(
- tableId, fileIO, manifestFile.path(), manifestFile.path(),
null, 1);
- })
- .get();
- return true;
- } catch (InterruptedException e) {
- LOGGER.error(
- "Interrupted exception deleting data files from manifest {}",
manifestFile.path(), e);
- throw new RuntimeException(e);
- } catch (ExecutionException e) {
- LOGGER.error("Unable to delete data files from manifest {}",
manifestFile.path(), e);
+ try (ManifestReader<DataFile> dataFiles = ManifestFiles.read(manifestFile,
fileIO, null)) {
+ List<CompletableFuture<Void>> dataFileDeletes =
+ StreamSupport.stream(
+ Spliterators.spliteratorUnknownSize(dataFiles.iterator(),
Spliterator.IMMUTABLE),
+ false)
+ .map(
+ file -> tryDelete(tableId, fileIO, manifestFile.path(),
file.location(), null, 1))
+ .toList();
+ LOGGER.debug(
+ "Scheduled {} data files to be deleted from manifest {}",
+ dataFileDeletes.size(),
+ manifestFile.path());
+ try {
+ // wait for all data files to be deleted, then wait for the manifest
itself to be deleted
+
CompletableFuture.allOf(dataFileDeletes.toArray(CompletableFuture[]::new))
+ .thenCompose(
+ (v) -> {
+ LOGGER
+ .atInfo()
+ .addKeyValue("manifestFile", manifestFile.path())
+ .log("All data files in manifest deleted - deleting
manifest");
+ return tryDelete(
+ tableId, fileIO, manifestFile.path(),
manifestFile.path(), null, 1);
+ })
+ .get();
+ return true;
+ } catch (InterruptedException e) {
+ LOGGER.error(
+ "Interrupted exception deleting data files from manifest {}",
manifestFile.path(), e);
+ throw new RuntimeException(e);
+ } catch (ExecutionException e) {
+ LOGGER.error("Unable to delete data files from manifest {}",
manifestFile.path(), e);
+ return false;
+ }
+ } catch (IOException e) {
+ LOGGER.error("Failed to close manifest reader for {}",
manifestFile.path(), e);
Review Comment:
Yeah you are right... The catch on the try-with will catch IO from the
`ManifestFiles.read()` call itself, from code inside the block (the iterator
etc), and from close(). The message was incorrectly assuming it was only a
close failure.
The try-with is the main thing that fixes the leak. The catch was there
because we want to return false instead of throwing, and because
`ManifestReader.close()` throws checked IOException.
I can update the message to be accurate (something like "**Failed to process
manifest reader**...") and add a comment or we can just drop the catch and let
it propagate if you prefer.
WDYT is better @dimas-b ?
--
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]
