dimas-b commented on code in PR #4828:
URL: https://github.com/apache/polaris/pull/4828#discussion_r3438266715
##########
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:
This is not completely correct. This `catch` gets all exceptions from the
`try` body too, I believe.
Do we really need to catch here?
--
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]
