github-actions[bot] commented on code in PR #66914: URL: https://github.com/apache/doris/pull/66914#discussion_r3840256041
########## fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/DorisHiveCatalog.java: ########## @@ -0,0 +1,77 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.iceberg; + +import org.apache.iceberg.hive.HiveCatalog; +import org.apache.iceberg.io.FileIO; + +import java.io.IOException; +import java.lang.reflect.Field; +import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; + +/** Owns and closes the shared FileIO created by one Iceberg HiveCatalog generation. */ +public class DorisHiveCatalog extends HiveCatalog { + private final AtomicBoolean closed = new AtomicBoolean(); + private FileIO ownedFileIO; + + @Override + public void initialize(String name, Map<String, String> properties) { + super.initialize(name, properties); + ownedFileIO = extractFileIO(); + } + + @Override + public void close() throws IOException { + if (!closed.compareAndSet(false, true)) { + return; + } + IOException closeFailure = null; + try { + super.close(); Review Comment: [P2] Guarantee FileIO cleanup when the upstream catalog close fails. Iceberg's BaseMetastoreCatalog.close() directly invokes the configured MetricsReporter.close(), and a RuntimeException from that extension escapes this IOException-only catch before ownedFileIO.close() runs. Because closed was already set, every retry is then a no-op and the FileIO remains unclosed. Please aggregate checked and unchecked failures from super.close() while still closing the FileIO, suppress any secondary cleanup failure, and cover this with a reporter-close-failure test. ########## fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/DorisHiveCatalog.java: ########## @@ -0,0 +1,77 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.iceberg; + +import org.apache.iceberg.hive.HiveCatalog; +import org.apache.iceberg.io.FileIO; + +import java.io.IOException; +import java.lang.reflect.Field; +import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; + +/** Owns and closes the shared FileIO created by one Iceberg HiveCatalog generation. */ +public class DorisHiveCatalog extends HiveCatalog { + private final AtomicBoolean closed = new AtomicBoolean(); + private FileIO ownedFileIO; + + @Override + public void initialize(String name, Map<String, String> properties) { + super.initialize(name, properties); Review Comment: [P2] Close the FileIO when Hive catalog initialization fails. In Iceberg 1.10.1, HiveCatalog.initialize creates and initializes the configured io-impl before constructing CachedClientPool; for example, client-pool-cache-keys=bogus makes that later constructor throw. Since ownedFileIO is assigned only after super.initialize returns and CatalogUtil does not close a catalog whose initialize call fails, each lazy retry can abandon a newly initialized FileIO and the clients it owns. Please capture/close the upstream FileIO on this failure path while preserving the initialization error, and add a partial-initialization test. -- 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]
