adutra commented on code in PR #8:
URL: https://github.com/apache/polaris-tools/pull/8#discussion_r2059905086
##########
polaris-synchronizer/api/src/main/java/org/apache/polaris/tools/sync/polaris/PolarisSynchronizer.java:
##########
@@ -665,6 +665,20 @@ public void syncCatalogs() {
syncNamespaces(
catalog.getName(), Namespace.empty(), sourceIcebergCatalogService,
targetIcebergCatalogService);
+ try {
Review Comment:
This looks suspicious; the catalogs won't be properly closed if an error is
thrown above.
Suggestion:
```java
try(IcebergCatalogService sourceIcebergCatalogService =
source.initializeIcebergCatalogService(catalog.getName())) {
clientLogger.info(
"Initialized Iceberg REST catalog for Polaris catalog {} on
source.",
catalog.getName());
try(IcebergCatalogService targetIcebergCatalogService =
target.initializeIcebergCatalogService(catalog.getName())) {
clientLogger.info(
"Initialized Iceberg REST catalog for Polaris catalog {} on
target.",
catalog.getName());
syncNamespaces(
catalog.getName(), Namespace.empty(),
sourceIcebergCatalogService, targetIcebergCatalogService);
}
} catch (Exception e) {
if (haltOnFailure) throw new RuntimeException(e);
clientLogger.error(
"Failed to synchronize Iceberg REST catalog for Polaris catalog
{}.",
catalog.getName(),
e);
continue;
}
```
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
