yyanyy commented on a change in pull request #2878:
URL: https://github.com/apache/iceberg/pull/2878#discussion_r678588138
##########
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:
Currently for all six catalog implementations, only `HiveCatalog` does
not implement Closeable/AutoCloseable, so I was thinking it may be fine to
extend it directly from the base class, and as we are not letting the interface
`Catalog` to implement closeable so I don't consider it as part of the
interface contract/design. I agree with composition over inheritance, and the
main reason for the current approach was mainly because I noticed this existing
`CloseableGroup` that does a very similar job as the goal I tried to achieve
here. In terms of the impact to other catalogs, all other non-hive and non-aws
catalogs implement `close()` explicitly which would override the logic of
`close()` in `CloseableGroup`, so I think there wouldn't be impact to the
current behavior; also `CloseableGroup` requires registration of closeable
resources in order to work, so even if a catalog does not implement `close()` I
don't think it would result in behavior change either.
I think the only thing that may be impacted by this is the nessie catalog -
currently nessie catalog extends `AutoCloseable` that in throws `Exception` in
`close()` signature, but now as we extends `CloseableGroup` in base, I believe
it will be extending `Closeable` (which extends `AutoCloseable`) which would
throw `IOException` in `close()` (this is the only difference in terms of
method signature between closeable and auto-closeable). However in the
meanwhile, nessie catalog [doesn't throw any checked
exception](https://github.com/apache/iceberg/blob/master/nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java#L129)
in `close()` at all, so I believe the code behavior wouldn't change either.
But please let me know if you think we should continue to let nessie catalog to
implement `AutoCloseable`, and in this case I can create a separate utility
class and only use them for aws related catalogs.
--
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]
