dimas-b commented on code in PR #4828:
URL: https://github.com/apache/polaris/pull/4828#discussion_r3443773914
##########
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:
The fact that `IOException` is not thrown in the `try` body is not apparent.
So the current message is correct, but it's not easy to see that from the code
😅
I'd prefer updating the message to be more generic, in case `IOException`
happens to be thrown in the `try` body later (due to future changes).
--
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]
