rymurr commented on a change in pull request #2878:
URL: https://github.com/apache/iceberg/pull/2878#discussion_r678973697
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -25,13 +25,14 @@
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.io.CloseableGroup;
import org.apache.iceberg.relocated.com.google.common.base.MoreObjects;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public abstract class BaseMetastoreCatalog implements Catalog {
+public abstract class BaseMetastoreCatalog extends CloseableGroup implements
Catalog {
Review comment:
I don't think this should be implemented by inheritance. `BaseCatalog`
provides utilities to help orchestrate a `Catalog` and does not deal with
lifetimes. I don't think its the responsibility of the abstract class to deal
w/ lifetimes. This is a concern of the individual catalogs imho. It shoud be
relatively clean to instaitate a closeablegroup in the constructor of a catalog
and close it in the catalogs close method.
We then are reducing the side effects for other catalog implementers and not
adding unneccessary contracts for catalog implementers.
--
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]
