This is an automated email from the ASF dual-hosted git repository. morningman pushed a commit to branch master-catalog-spi-review-21 in repository https://gitbox.apache.org/repos/asf/doris.git
commit 18cc4518ea0a5399350ac6b5d48a00ecd73aea02 Author: morningman <[email protected]> AuthorDate: Tue Jul 28 14:50:52 2026 +0800 [refactor](catalog) fe-core: retire the metastore property cluster Deletes org.apache.doris.datasource.property.metastore (4 files) and the ConnectionProperties base it orphans. Nothing moves: the successor is already live connector-side (fe-connector-metastore-api MetaStoreProperties, fe-connector-metastore-spi MetaStoreProviders.bind, and Connector.deriveStorageProperties), so this is deletion, not migration. The cluster was unreachable. Its registry held only TRINO_CONNECTOR, whose factory returns a bare MetastoreProperties without ever calling initNormalizeAndCheckProps -- so it parsed nothing, and createInternal, getDerivedStorageProperties, getExecutionAuthenticator and the StorageAuthenticatorBridge all had zero callers. CatalogProperty was the sole importer and offered two doors: checkMetaStoreAndStorageProperties (no callers repo-wide) and resolveDerivedStorageDefaults, whose metastore branch runs only when pluginDerivedStorageDefaultsSupplier is null -- and PluginDrivenExternalCatalog installs that supplier unconditionally. The other two ExternalCatalog subclasses left in fe-core never touch the storage path. Nothing here was Gson-persisted, so no image or replay compatibility is at stake; also note MetastoreProperties.Type is unrelated to the persisted InitCatalogLog.Type/InitDatabaseLog.Type constants of the same name. resolveDerivedStorageDefaults now throws when the supplier is unwired instead of deriving nothing. That preserves today's behavior, where the metastore parse threw on this path. Deriving an empty map would silently drop the iceberg warehouse -> fs.defaultFS bridge and, because the setter deliberately does not reset caches, would cache the under-derived StorageBindings for good. CatalogPropertyPluginStorageDerivationTest gains a case pinning that, and its stale mutation comments are retargeted at mutations that still exist. Three dangling comment references are reworded. The one in StorageAdapter is javadoc prose, not a compile dependency. Verified: full reactor clean test-compile including test sources; fe-core checkstyle 0 violations; 95 fe-core tests including the Gson replay suites; 110 fe-connector-api tests with connector-metadata-methods.txt unchanged. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../paimon/TcclPinningConnectorContext.java | 2 +- .../doris/common/util/DatasourcePrintableMap.java | 4 +- .../apache/doris/datasource/CatalogProperty.java | 97 +++-------- .../datasource/property/ConnectionProperties.java | 140 --------------- .../AbstractMetastorePropertiesFactory.java | 74 -------- .../property/metastore/MetastoreProperties.java | 191 --------------------- .../metastore/MetastorePropertiesFactory.java | 36 ---- .../metastore/TrinoConnectorPropertiesFactory.java | 32 ---- .../doris/datasource/storage/StorageAdapter.java | 7 +- ...CatalogPropertyPluginStorageDerivationTest.java | 33 +++- plan-doc/fecore-property-cleanup/HANDOFF.md | 105 ++++++----- plan-doc/fecore-property-cleanup/open-decisions.md | 8 +- plan-doc/fecore-property-cleanup/progress.md | 57 ++++++ plan-doc/fecore-property-cleanup/tasklist.md | 24 ++- 14 files changed, 180 insertions(+), 630 deletions(-) diff --git a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/TcclPinningConnectorContext.java b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/TcclPinningConnectorContext.java index aa12b062845..cf5578c15fc 100644 --- a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/TcclPinningConnectorContext.java +++ b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/TcclPinningConnectorContext.java @@ -46,7 +46,7 @@ import java.util.function.Supplier; * {@link HadoopAuthenticator} and the op runs inside its {@code doAs}. This is REQUIRED because the plugin * bundles its own {@code hadoop-common} + {@code fe-kerberos} child-first, so the plugin's HDFS * {@code FileSystem} reads a DIFFERENT {@code UserGroupInformation} copy than the one the FE-injected - * authenticator (built app-side by the fe-core {@code MetastoreProperties}) logs in — the app-side + * authenticator (built app-side, outside the plugin loader) logs in — the app-side * {@code doAs} therefore never reaches the plugin FileSystem, which falls back to SIMPLE auth. The connector * is the only party that knows which UGI copy its FileSystem uses, so it owns the auth: on the Kerberos path * we run the plugin {@code doAs} and DELIBERATELY do NOT also call {@code delegate.executeAuthenticated} diff --git a/fe/fe-core/src/main/java/org/apache/doris/common/util/DatasourcePrintableMap.java b/fe/fe-core/src/main/java/org/apache/doris/common/util/DatasourcePrintableMap.java index f80326e1c83..3a5e2ed3c2a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/common/util/DatasourcePrintableMap.java +++ b/fe/fe-core/src/main/java/org/apache/doris/common/util/DatasourcePrintableMap.java @@ -66,8 +66,8 @@ public class DatasourcePrintableMap<K, V> extends BasicPrintableMap<K, V> { // authoritative copy now lives connector-side (fe-connector-metastore-iceberg // IcebergRestMetaStoreProperties), which fe-core cannot depend on. SHOW CREATE CATALOG masking must // still hide these, so all four former IcebergRestProperties sensitive keys are enumerated explicitly, - // byte-identical to the former reflection result (its AbstractIcebergProperties/MetastoreProperties - // superclass chain carries no sensitive keys). Note the overlap with the inlined storage keys below is + // byte-identical to the former reflection result (nothing in its now-retired fe-core superclass chain + // carried sensitive keys). Note the overlap with the inlined storage keys below is // uneven and must NOT be relied on: iceberg.rest.secret-access-key aliases the (sensitive) S3 secret // key, but iceberg.rest.session-token aliases the S3 session-token field which is NOT sensitive, so // omitting it here would silently unmask it. Keep in sync with the connector's sensitive REST keys. diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java index fa69133e6f4..2ad75d6b950 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogProperty.java @@ -17,19 +17,13 @@ package org.apache.doris.datasource; -import org.apache.doris.common.UserException; -import org.apache.doris.datasource.property.metastore.MetastoreProperties; import org.apache.doris.datasource.storage.StorageAdapter; import org.apache.doris.datasource.storage.StorageTypeId; -import com.google.common.base.Preconditions; import com.google.common.collect.Maps; import com.google.gson.annotations.SerializedName; import org.apache.commons.collections4.MapUtils; -import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.hadoop.conf.Configuration; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import java.util.Collections; import java.util.HashMap; @@ -45,8 +39,6 @@ import java.util.stream.Collectors; * the properties in "properties" will overwrite properties in "resource" */ public class CatalogProperty { - private static final Logger LOG = LogManager.getLogger(CatalogProperty.class); - // Default: false, mapping BINARY types to STRING for compatibility public static final String ENABLE_MAPPING_VARBINARY = "enable.mapping.varbinary"; // Default: false, mapping TIMESTAMP_TZ types to DATETIME for compatibility @@ -90,9 +82,6 @@ public class CatalogProperty { } } - // Lazy-loaded metastore properties, using volatile to ensure visibility - private volatile MetastoreProperties metastoreProperties; - // Lazy-loaded backend storage properties, using volatile to ensure visibility private volatile Map<String, String> backendStorageProperties; @@ -101,10 +90,11 @@ public class CatalogProperty { // Design S8: for a plugin catalog, the connector owns storage-property derivation (e.g. iceberg hadoop // warehouse -> fs.defaultFS); this supplier returns the connector-derived storage defaults fe-core folds - // into the storage map, so fe-core does NOT parse metastore properties on the storage path. Null for a - // legacy catalog (Hive/Hudi/HMS-iceberg/LakeSoul), which derives from its fe-core MetastoreProperties. Set - // once by PluginDrivenExternalCatalog after the connector is created; deliberately NOT cleared by - // resetAllCaches (it is catalog wiring, not a derived cache, and an ALTER re-runs catalog init anyway). + // into the storage map, so fe-core does NOT parse metastore properties on the storage path. Set once by + // PluginDrivenExternalCatalog after the connector is created; deliberately NOT cleared by resetAllCaches + // (it is catalog wiring, not a derived cache, and an ALTER re-runs catalog init anyway). Every catalog that + // reaches the storage path is plugin-driven, so a null supplier here is a wiring bug, not a legacy flavor -- + // see resolveDerivedStorageDefaults. private volatile Supplier<Map<String, String>> pluginDerivedStorageDefaultsSupplier; public CatalogProperty(String resource, Map<String, String> properties) { @@ -186,7 +176,6 @@ public class CatalogProperty { */ private void resetAllCaches() { this.storageBindings = null; - this.metastoreProperties = null; this.backendStorageProperties = null; this.hadoopProperties = null; } @@ -236,10 +225,9 @@ public class CatalogProperty { } /** - * The catalog's persisted user props merged with derived storage defaults. For a plugin catalog the - * connector supplies them (design S8: {@link #pluginDerivedStorageDefaultsSupplier} — fe-core does not - * parse metastore properties for storage); for a legacy catalog they come from its fe-core - * {@link MetastoreProperties}. Derived props are defaults (an explicit user key wins via {@code putIfAbsent}) + * The catalog's persisted user props merged with derived storage defaults, which the connector supplies + * (design S8: {@link #pluginDerivedStorageDefaultsSupplier} — fe-core does not parse metastore properties + * for storage). Derived props are defaults (an explicit user key wins via {@code putIfAbsent}) * and the persisted {@link #getProperties()} map is never mutated. This is the exact map * {@link #initStorageAdapters} feeds to {@link StorageAdapter#ofAll} and that * {@link #getEffectiveRawStorageProperties} hands the connector for fe-filesystem binding. @@ -255,26 +243,29 @@ public class CatalogProperty { } /** - * Resolves the derived storage defaults. A plugin catalog gets them from the connector (design S8 — fe-core - * does not parse metastore properties for storage; the iceberg connector bridges its hadoop warehouse to - * {@code fs.defaultFS}). A legacy catalog derives them from its fe-core {@link MetastoreProperties}, which - * is empty for every remaining type once the iceberg cluster's sole {@code getDerivedStorageProperties} - * override moved to the connector. + * Resolves the derived storage defaults from the connector (design S8 — fe-core does not parse metastore + * properties for storage; the iceberg connector bridges its hadoop warehouse to {@code fs.defaultFS}). + * + * <p>A null supplier means storage was accessed before {@link #setPluginDerivedStorageDefaultsSupplier} + * ran, which no path does today: every catalog that reaches the storage path is plugin-driven, and no + * connector touches storage while it is being constructed. Fail loud rather than silently deriving + * nothing — a silent empty map would drop the {@code warehouse -> fs.defaultFS} bridge AND cache the + * under-derived {@link StorageBindings} for good, since the setter deliberately does not reset caches. + * This preserves the previous behavior, where the retired fe-core metastore parse threw here.</p> */ private Map<String, String> resolveDerivedStorageDefaults() { Supplier<Map<String, String>> pluginSupplier = pluginDerivedStorageDefaultsSupplier; - if (pluginSupplier != null) { - Map<String, String> derived = pluginSupplier.get(); - return derived != null ? derived : Collections.emptyMap(); + if (pluginSupplier == null) { + throw new IllegalStateException("Storage properties were accessed before the connector-derived " + + "storage defaults were wired for catalog properties: " + properties.keySet()); } - MetastoreProperties msp = getMetastoreProperties(); - return msp != null ? msp.getDerivedStorageProperties() : Collections.emptyMap(); + Map<String, String> derived = pluginSupplier.get(); + return derived != null ? derived : Collections.emptyMap(); } /** - * Design S8: wires the connector-owned storage-derivation source for a plugin catalog. Once set, storage - * property init folds the connector-derived defaults instead of deriving from fe-core MetastoreProperties, - * so a plugin iceberg/paimon catalog never calls {@link #getMetastoreProperties()} on the storage path. + * Design S8: wires the connector-owned storage-derivation source for a plugin catalog. Must be set before + * anything reads storage properties — {@link #resolveDerivedStorageDefaults()} throws otherwise. */ public void setPluginDerivedStorageDefaultsSupplier(Supplier<Map<String, String>> supplier) { this.pluginDerivedStorageDefaultsSupplier = supplier; @@ -304,46 +295,6 @@ public class CatalogProperty { } } - public void checkMetaStoreAndStorageProperties(Class msClass) { - MetastoreProperties msProperties; - try { - msProperties = MetastoreProperties.create(getProperties()); - initStorageAdapters(); - } catch (UserException e) { - throw new RuntimeException("Failed to initialize Catalog properties, error: " - + ExceptionUtils.getRootCauseMessage(e), e); - } - Preconditions.checkNotNull(msProperties, "Metastore properties are not configured properly"); - Preconditions.checkArgument( - msClass.isInstance(msProperties), - String.format("Metastore properties type is not correct. Expected %s but got %s", - msClass.getName(), msProperties.getClass().getName())); - } - - /** - * Get metastore properties with lazy loading, using double-check locking to ensure thread safety - */ - public MetastoreProperties getMetastoreProperties() { - if (MapUtils.isEmpty(getProperties())) { - return null; - } - - if (metastoreProperties == null) { - synchronized (this) { - if (metastoreProperties == null) { - try { - metastoreProperties = MetastoreProperties.create(getProperties()); - } catch (UserException e) { - LOG.warn("Failed to create metastore properties", e); - throw new RuntimeException("Failed to create metastore properties, error: " - + ExceptionUtils.getRootCauseMessage(e), e); - } - } - } - } - return metastoreProperties; - } - /** * Get backend storage properties with lazy loading, using double-check locking to ensure thread safety */ diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/ConnectionProperties.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/ConnectionProperties.java deleted file mode 100644 index 7a18e1e6908..00000000000 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/ConnectionProperties.java +++ /dev/null @@ -1,140 +0,0 @@ -// 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.datasource.property; - -import org.apache.doris.common.CatalogConfigFileUtils; -import org.apache.doris.foundation.property.ConnectorPropertiesUtils; -import org.apache.doris.foundation.property.ConnectorProperty; -import org.apache.doris.foundation.property.StoragePropertiesException; - -import com.google.common.base.Strings; -import com.google.common.collect.Maps; -import lombok.Getter; -import lombok.Setter; -import org.apache.hadoop.conf.Configuration; - -import java.lang.reflect.Field; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Objects; - -public abstract class ConnectionProperties { - /** - * The original user-provided properties. - * <p> - * This map may contain various configuration entries, not all of which are relevant - * to the specific Connector implementation. It serves as the raw input from the user. - */ - @Getter - @Setter - protected Map<String, String> origProps; - - /** - * The filtered properties that are actually used by the Connector. - * <p> - * This map only contains key-value pairs that are recognized and matched by - * the specific Connector implementation. It's a subset of {@code origProps}. - */ - @Getter - protected Map<String, String> matchedProperties = new HashMap<>(); - - protected ConnectionProperties(Map<String, String> origProps) { - this.origProps = origProps; - } - - public void initNormalizeAndCheckProps() { - ConnectorPropertiesUtils.bindConnectorProperties(this, origProps); - for (Field field : ConnectorPropertiesUtils.getConnectorProperties(this.getClass())) { - ConnectorProperty annotation = field.getAnnotation(ConnectorProperty.class); - for (String name : annotation.names()) { - if (origProps.containsKey(name)) { - matchedProperties.put(name, origProps.get(name)); - break; - } - } - } - // 3. check properties - checkRequiredProperties(); - } - - // Some properties may be loaded from file - // Subclass can override this method to load properties from file. - // The return value is the properties loaded from file, not include original properties - protected Map<String, String> loadConfigFromFile(String resourceConfig) { - if (Strings.isNullOrEmpty(resourceConfig)) { - return new HashMap<>(); - } - Configuration conf = CatalogConfigFileUtils.loadConfigurationFromHadoopConfDir(resourceConfig); - Map<String, String> confMap = Maps.newHashMap(); - for (Map.Entry<String, String> entry : conf) { - confMap.put(entry.getKey(), entry.getValue()); - } - return confMap; - } - - // Subclass can override this method to return the property name of resource config. - protected String getResourceConfigPropName() { - return null; - } - - // This method will check if all required properties are set. - // Subclass can implement this method for additional check. - protected void checkRequiredProperties() { - List<Field> supportedProps = ConnectorPropertiesUtils.getConnectorProperties(this.getClass()); - for (Field field : supportedProps) { - field.setAccessible(true); - ConnectorProperty anno = field.getAnnotation(ConnectorProperty.class); - String[] names = anno.names(); - if (anno.required() && field.getType().equals(String.class)) { - try { - String value = (String) field.get(this); - if (Strings.isNullOrEmpty(value)) { - throw new IllegalArgumentException("Property " + names[0] + " is required."); - } - } catch (IllegalAccessException e) { - throw new StoragePropertiesException("Failed to get property " + names[0] - + ", " + e.getMessage(), e); - } - } - } - } - - /** - * Two ConnectionProperties are equal if they are of the same concrete type and - * have the same original properties. This ensures that logically identical - * configurations share the same cache key in {@code FileSystemCache}, preventing - * cache entry duplication and use-after-eviction race conditions. - */ - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (obj == null || getClass() != obj.getClass()) { - return false; - } - ConnectionProperties other = (ConnectionProperties) obj; - return Objects.equals(origProps, other.origProps); - } - - @Override - public int hashCode() { - return Objects.hash(getClass().getName(), origProps); - } -} diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractMetastorePropertiesFactory.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractMetastorePropertiesFactory.java deleted file mode 100644 index 581b18e9d50..00000000000 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/AbstractMetastorePropertiesFactory.java +++ /dev/null @@ -1,74 +0,0 @@ -// 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.datasource.property.metastore; - -import java.util.HashMap; -import java.util.Locale; -import java.util.Map; -import java.util.function.Function; - -/** - * Abstract base class for implementing {@link MetastorePropertiesFactory}. - * <p> - * This class provides common logic for: - * - Registering metastore subtypes. - * - Selecting the appropriate constructor based on user-specified properties. - * <p> - * Subclasses only need to register their supported subtypes and provide the corresponding config key. - */ -public abstract class AbstractMetastorePropertiesFactory implements MetastorePropertiesFactory { - - protected final Map<String, Function<Map<String, String>, MetastoreProperties>> registeredSubTypes = - new HashMap<>(); - - /** - * Registers a new metastore subtype with its corresponding constructor. - * - * @param subType the subtype name (e.g., "hms", "glue") - * @param constructor the function that creates the {@link MetastoreProperties} instance - */ - protected void register(String subType, Function<Map<String, String>, MetastoreProperties> constructor) { - registeredSubTypes.put(subType.toLowerCase(Locale.ROOT), constructor); - } - - /** - * Creates a {@link MetastoreProperties} instance based on the specified properties and subtype key. - * - * @param props the configuration map - * @param key the property key used to determine the subtype (e.g., "hive.metastore.type") - * @param defaultType the default subtype to fall back on if the key is not present (nullable) - * @return a properly initialized {@link MetastoreProperties} instance - * @throws IllegalArgumentException if the subtype is missing, empty, or unsupported - */ - protected MetastoreProperties createInternal(Map<String, String> props, String key, String defaultType) { - String subType = props.getOrDefault(key, defaultType); - if (subType == null || subType.trim().isEmpty()) { - throw new IllegalArgumentException(key + " is not set or is empty in properties"); - } - - Function<Map<String, String>, MetastoreProperties> constructor = - registeredSubTypes.get(subType.toLowerCase(Locale.ROOT)); - if (constructor == null) { - throw new IllegalArgumentException("Unsupported metastore subtype: " + subType); - } - - MetastoreProperties instance = constructor.apply(props); - instance.initNormalizeAndCheckProps(); - return instance; - } -} diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/MetastoreProperties.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/MetastoreProperties.java deleted file mode 100644 index 93787be1d19..00000000000 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/MetastoreProperties.java +++ /dev/null @@ -1,191 +0,0 @@ -// 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.datasource.property.metastore; - -import org.apache.doris.common.UserException; -import org.apache.doris.datasource.property.ConnectionProperties; -import org.apache.doris.kerberos.ExecutionAuthenticator; - -import lombok.Getter; -import org.apache.commons.lang3.StringUtils; - -import java.util.Arrays; -import java.util.Collections; -import java.util.EnumMap; -import java.util.HashSet; -import java.util.Locale; -import java.util.Map; -import java.util.Optional; -import java.util.Set; - -/** - * MetastoreProperties is the base class for handling configuration of different types of metastores - * such as Hive Metastore (HMS), AWS Glue, Aliyun DLF, Iceberg REST catalog, Google Dataproc, - * or file-based metastores (like Hadoop). - * <p> - * It uses a simple factory pattern based on a registry to dynamically instantiate the correct - * subclass according to the provided configuration. - * <p> - * Supported metastore types are defined in the {@link Type} enum. Multiple alias names can be mapped to each type. - */ -public class MetastoreProperties extends ConnectionProperties { - - public enum Type { - HMS("hms"), - ICEBERG("iceberg"), - PAIMON("paimon"), - GLUE("glue"), - DLF("dlf"), - DATAPROC("dataproc"), - FILE_SYSTEM("filesystem", "hadoop"), - TRINO_CONNECTOR("trino-connector"), - UNKNOWN(); - - private final Set<String> aliases; - - Type(String... aliases) { - this.aliases = new HashSet<>(Arrays.asList(aliases)); - } - - public static Optional<Type> fromString(String input) { - if (input == null) { - return Optional.empty(); - } - String normalized = input.trim().toLowerCase(Locale.ROOT); - for (Type type : values()) { - if (type.aliases.contains(normalized)) { - return Optional.of(type); - } - } - return Optional.empty(); - } - } - - @Getter - protected Type type; - - private static final String METASTORE_TYPE_KEY = "type"; - - private static final Map<Type, MetastorePropertiesFactory> FACTORY_MAP = new EnumMap<>(Type.class); - - static { - //subclasses should be registered here - // Design S7: hms/iceberg/paimon are plugin (SPI) catalogs whose metastore properties live - // connector-side; fe-core no longer parses them. The Type.HMS/ICEBERG/PAIMON enum values remain (so a - // stray create() fails loud with "Unsupported metastore type") but their factories are intentionally - // not registered. - register(Type.TRINO_CONNECTOR, new TrinoConnectorPropertiesFactory()); - } - - public static void register(Type type, MetastorePropertiesFactory factory) { - FACTORY_MAP.put(type, factory); - } - - public static MetastoreProperties create(Map<String, String> props) throws UserException { - Type type = resolveType(props); - MetastorePropertiesFactory factory = FACTORY_MAP.get(type); - if (factory == null) { - throw new IllegalArgumentException("Unsupported metastore type: " + type); - } - return factory.create(props); - } - - private static Type resolveType(Map<String, String> props) { - String typeValue = props.get(METASTORE_TYPE_KEY); - if (StringUtils.isBlank(typeValue)) { - throw new IllegalArgumentException("Metastore type is required"); - } - - Optional<Type> typeOpt = Type.fromString(typeValue); - if (typeOpt.isPresent()) { - return typeOpt.get(); - } - throw new IllegalArgumentException("Unknown metastore type value '" + typeValue + "'. " - + "Supported types are: " + Arrays.toString(Type.values())); - } - - protected MetastoreProperties(Type type, Map<String, String> props) { - super(props); - this.type = type; - } - - protected MetastoreProperties(Map<String, String> props) { - super(props); - } - - /** - * Returns the execution authenticator for this metastore. - * Subclasses that support Kerberos override this via their - * {@code @Getter executionAuthenticator} field to return a Kerberos-capable authenticator. - * - * <p>The default implementation returns a simple no-op authenticator.</p> - */ - private static final ExecutionAuthenticator NOOP_AUTH = new ExecutionAuthenticator() {}; - - public ExecutionAuthenticator getExecutionAuthenticator() { - return NOOP_AUTH; - } - - /** - * Storage-configuration properties derived from this metastore's own properties that the raw catalog - * property map does not already supply. {@code CatalogProperty.initStorageProperties} merges them (as - * defaults — an explicit user key always wins) into the map fed to {@code StorageProperties.createAll}, - * before storage-backend detection. - * - * <p>The default is empty: no derivation, zero behavior change for every existing metastore type. The - * iceberg filesystem flavor overrides it to bridge a {@code warehouse=hdfs://<ns>/path} into - * {@code fs.defaultFS=hdfs://<ns>} — legacy {@code IcebergHadoopExternalCatalog} did this in its - * constructor (dead on the plugin/cutover path), and the shared HDFS detection never reads - * {@code warehouse}, so an HA-nameservice hadoop catalog configured with only {@code warehouse} would - * otherwise fail to bind HDFS storage.</p> - */ - public Map<String, String> getDerivedStorageProperties() { - return Collections.emptyMap(); - } - - /** - * Bridges the storage facade's foundation-typed authenticator into the legacy fe-common - * {@link ExecutionAuthenticator} consumer surface. Temporary migration shim shared by the - * Paimon and Iceberg metastore families: once the metastore consumer track re-types - * {@code executionAuthenticator} to the foundation interface, facade authenticators can be - * assigned directly and this helper disappears. - */ - protected static ExecutionAuthenticator asLegacyAuthenticator( - org.apache.doris.foundation.security.ExecutionAuthenticator delegate) { - return new StorageAuthenticatorBridge(delegate); - } - - /** Named (test-assertable) fe-common view over a foundation authenticator. */ - public static final class StorageAuthenticatorBridge implements ExecutionAuthenticator { - private final org.apache.doris.foundation.security.ExecutionAuthenticator delegate; - - StorageAuthenticatorBridge(org.apache.doris.foundation.security.ExecutionAuthenticator delegate) { - this.delegate = delegate; - } - - @Override - public <T> T execute(java.util.concurrent.Callable<T> task) throws Exception { - return delegate.execute(task); - } - - @Override - public void execute(Runnable task) throws Exception { - delegate.execute(task); - } - } -} diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/MetastorePropertiesFactory.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/MetastorePropertiesFactory.java deleted file mode 100644 index 49748804f30..00000000000 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/MetastorePropertiesFactory.java +++ /dev/null @@ -1,36 +0,0 @@ -// 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.datasource.property.metastore; - -import org.apache.doris.common.UserException; - -import java.util.Map; - -/** - * A factory interface for creating {@link MetastoreProperties} instances based on user-defined properties. - * <p> - * In general, the metastore type of a catalog follows a two-level structure: - * - The first-level type is determined by the catalog type (e.g., "hive", "iceberg"). - * - The second-level type is a subtype that needs to be registered individually (e.g., "hms"). - * <p> - * Each catalog type should have its own implementation of this factory interface, - * with its supported subtypes registered internally. - */ -public interface MetastorePropertiesFactory { - MetastoreProperties create(Map<String, String> props) throws UserException; -} diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/TrinoConnectorPropertiesFactory.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/TrinoConnectorPropertiesFactory.java deleted file mode 100644 index 5bdbafc58bf..00000000000 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/property/metastore/TrinoConnectorPropertiesFactory.java +++ /dev/null @@ -1,32 +0,0 @@ -// 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.datasource.property.metastore; - -import org.apache.doris.datasource.property.metastore.MetastoreProperties.Type; - -import java.util.Map; - -/** - * Just a placeholder - */ -public class TrinoConnectorPropertiesFactory extends AbstractMetastorePropertiesFactory { - @Override - public MetastoreProperties create(Map<String, String> props) { - return new MetastoreProperties(Type.TRINO_CONNECTOR, props); - } -} diff --git a/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/StorageAdapter.java b/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/StorageAdapter.java index 13f42b5de05..b2c582b6518 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/StorageAdapter.java +++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/storage/StorageAdapter.java @@ -852,10 +852,9 @@ public final class StorageAdapter { } /** - * Value equality over (provider, raw properties, broker-name override), mirroring the legacy - * {@code ConnectionProperties.equals}: logically identical configurations must share one - * {@code FileSystemCache} key so equal-config rebinds (e.g. catalog property rollback) - * re-hit cached filesystems instead of duplicating them. + * Value equality over (provider, raw properties, broker-name override): logically identical + * configurations must share one {@code FileSystemCache} key so equal-config rebinds (e.g. catalog + * property rollback) re-hit cached filesystems instead of duplicating them. */ @Override public boolean equals(Object other) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogPropertyPluginStorageDerivationTest.java b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogPropertyPluginStorageDerivationTest.java index 3c4c304534c..a66ee0323d3 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogPropertyPluginStorageDerivationTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/datasource/CatalogPropertyPluginStorageDerivationTest.java @@ -26,12 +26,12 @@ import java.util.HashMap; import java.util.Map; /** - * Design S8: for a plugin catalog the connector owns storage-property derivation, so {@link CatalogProperty} - * folds the connector-supplied defaults (via {@link CatalogProperty#setPluginDerivedStorageDefaultsSupplier}) + * Design S8: the connector owns storage-property derivation, so {@link CatalogProperty} folds the + * connector-supplied defaults (via {@link CatalogProperty#setPluginDerivedStorageDefaultsSupplier}) * into BOTH the raw fe-filesystem bind map ({@link CatalogProperty#getEffectiveRawStorageProperties}) and the - * typed BE storage map ({@link CatalogProperty#getStorageAdaptersMap}) WITHOUT parsing fe-core - * {@code MetastoreProperties}. This is what lets the fe-core Iceberg/Paimon MetastoreProperties cluster be - * retired (their factory is un-registered, so any getMetastoreProperties() on the plugin path would throw). + * typed BE storage map ({@link CatalogProperty#getStorageAdaptersMap}). This is the sole guard on that path + * now that the fe-core metastore-property cluster is retired: fe-core has no second way to derive storage + * defaults, so an unwired supplier must fail loud rather than silently derive nothing. */ public class CatalogPropertyPluginStorageDerivationTest { @@ -48,10 +48,10 @@ public class CatalogPropertyPluginStorageDerivationTest { @Test public void pluginSupplierFoldsDerivedDefaultsIntoBothMaps() { CatalogProperty cp = hadoopIcebergCatalog(); - // A value the fe-core metastore path would NOT produce (that path derives hdfs://realns from warehouse): - // asserting hdfs://from-connector proves the plugin path uses the connector supplier and does not - // re-derive from MetastoreProperties. MUTATION: route resolveDerivedStorageDefaults back through - // getMetastoreProperties() -> value becomes hdfs://realns -> red. + // hdfs://from-connector is deliberately NOT derivable from any property below (a warehouse bridge + // would yield hdfs://realns), so asserting it proves the value travelled through the connector + // supplier. MUTATION: drop the derived-defaults fold in mergeDerivedStorageDefaults (or have + // resolveDerivedStorageDefaults ignore the supplier) -> both assertions go null -> red. cp.setPluginDerivedStorageDefaultsSupplier( () -> Collections.singletonMap("fs.defaultFS", "hdfs://from-connector")); // Raw supplier (fe-filesystem bind path). @@ -86,4 +86,19 @@ public class CatalogPropertyPluginStorageDerivationTest { Assertions.assertFalse(cp.getProperties().containsKey("fs.defaultFS"), "persisted props must not gain the derived fs.defaultFS"); } + + @Test + public void unwiredSupplierFailsLoudInsteadOfDerivingNothing() { + // Retiring the fe-core metastore parse left the connector supplier as the ONLY derivation source, so + // reading storage before it is wired can no longer fall back to anything. It must throw: silently + // deriving nothing would drop the warehouse -> fs.defaultFS bridge AND cache the under-derived + // StorageBindings for good, because setPluginDerivedStorageDefaultsSupplier deliberately does not + // reset caches -- a later correct wiring would never repair it. No production path reaches this today + // (every catalog on the storage path is plugin-driven and no connector touches storage while being + // constructed); this pins that a future one fails visibly. + // MUTATION: return Collections.emptyMap() instead of throwing -> red. + CatalogProperty cp = hadoopIcebergCatalog(); + Assertions.assertThrows(IllegalStateException.class, cp::getEffectiveRawStorageProperties); + Assertions.assertThrows(IllegalStateException.class, cp::getStorageAdaptersMap); + } } diff --git a/plan-doc/fecore-property-cleanup/HANDOFF.md b/plan-doc/fecore-property-cleanup/HANDOFF.md index 8e40b1db753..677dfdb52bb 100644 --- a/plan-doc/fecore-property-cleanup/HANDOFF.md +++ b/plan-doc/fecore-property-cleanup/HANDOFF.md @@ -7,86 +7,77 @@ --- -# 🆕 下一个 session 第一件事 = **拿 OD-1 的拍板**（`open-decisions.md`） +# 🆕 下一个 session = **FPC-02**（删 AWS 死构造臂），先过 OD-2 -## 状态：**调研已完成，代码一行未动。** +## 状态：**主删除 FPC-03 已完成并验证通过。`metastore/` 目录已不存在。** -**基线 HEAD** = `3468d905eb3`（分支 `catalog-spi-review-21`，2026-07-28）。 -本空间是**新建**的，尚无任何本任务的 commit。 +| 阶段 | commit | 结果 | +|---|---|---| +| 文档空间 | `938d38c7425` | 6 份文档落盘 | +| FPC-01 + FPC-03 | 见 `git log` | **删 5 文件 473 行** + `CatalogProperty` 净减 ~45 行；全反应堆绿 / checkstyle 0 violations / 95+110 单测全过 | ---- +`fe/fe-core/.../datasource/property/` 现在只剩 `common` / `constants` / `fileformat`。 -## 📍 你现在需要知道的三件事 +--- -### 1️⃣ 两个包的答案不一样，别当成一件事做 +## ⏳ 一件待用户追认的事（**别忘了问**） -| 包 | 裁决 | -|---|---| -| `metastore/`（4 文件 333 行）+ `ConnectionProperties.java`（140 行） | **整体删除** —— 运行期不可达，接班人 `fe-connector-metastore-api` 早已上线，**没有东西需要搬** | -| `common/`（2 文件 237 行） | **留在 fe-core** —— 它服务的是**内部存储**（冷存 StoragePolicy / 云上 StorageVault），不是外部数据源；只砍掉死的一半 | +**OD-1 用户始终没表态**，我按文档推荐值 **A（fail-loud）** 落地了： +`CatalogProperty.resolveDerivedStorageDefaults()` 的 null-supplier 分支 +`throw new IllegalStateException(...)`，并配了守卫测试 +`CatalogPropertyPluginStorageDerivationTest.unwiredSupplierFailsLoudInsteadOfDerivingNothing` +（已做变异验证）。 -### 2️⃣ 🔴 一个必须记住的「别再犯」 +**要翻成 B（`return Collections.emptyMap()`）只需改一行 + 删该用例。** +开场时向用户确认一句即可，不必重新论证。 -调研初判说 `common/` 和 `fe-filesystem-s3-base` 的 -`S3CredentialsProviderType`/`S3CredentialsProviderFactory` 是重复造轮子、可以直接替换 —— -**这个判断被对抗验证两轮推翻了**。两份实现有**两条活的行为差异**（`design.md` §3.3）： +--- -1. 发给 hadoop 的凭证串会**多出** `ProfileCredentialsProvider` -2. 模式串接受面会**放宽**（空串 / `ENVIRONMENT` / `WEB_IDENTITY_TOKEN_FILE` 从抛异常变成接受） +## 📋 下一步：FPC-02（`tasklist.md` 阶段 2） -而**全仓没有任何测试钉住那个串** ⇒ 换掉会**绿着上线一个回归**。 -**下次看到「这两个类长得一样，合并掉吧」的念头，先来读 `design.md` §3.3。** +删 `StorageAdapter.getAwsCredentialsProvider()` + 两个私有 helper， +以及 `AwsCredentialsProviderFactory` 的 `createV2` / `createDefaultV2` / 单参 `getV2ClassName`， +共 **~146 行零调用者代码，零行为变更**。 -### 3️⃣ ⛔ 卡在哪：OD-1 没拍板 +**动手前先过 OD-2**：grep 一次上游 `apache/doris` master 看有没有 +`getAwsCredentialsProvider()` 的调用者（这段是上游 `f499c78c67c` / #66004 整体带进来的， +若上游有调用者，下次 rebase 会 modify/delete 冲突）。 -`open-decisions.md` **OD-1**：删掉 metastore 后，`resolveDerivedStorageDefaults()` 的 -null-supplier 分支要 **fail-loud（`throw`，推荐）** 还是 **fail-silent（`return emptyMap()`）**？ +⚠️ `tasklist.md` FPC-02 里的**「必须保留」清单要逐条对**—— +`getAwsCredentialsProviderMode()` 和 `s3CredentialsMode` 字段是**活的**（喂 BE 的 +`AWS_CREDENTIALS_PROVIDER_TYPE`，且被 `AzureGuessRoutingParityTest` 钉着），删了会炸。 -这条**直接决定 FPC-03 的代码怎么写**，不定不能开工。 -（OD-2 是「FPC-02 做不做」，不阻塞任何东西，可以随时定。） +🟢 FPC-02 **可以整项丢弃**，不影响已完成的 FPC-03。 --- -## ▶️ 建议的执行顺序 +## ⚠️ 四条验证纪律（**第 4 条是这轮实测新增的**） -``` -1. 拿 OD-1 拍板 ← 就是这一步，先做 -2. FPC-02（删 AWS 死构造臂，~146 行） ← 可选、解耦，先做也行、跳过也行 -3. FPC-03（主删除：5 文件 473 行 + CatalogProperty 瘦身 ~45 行） ← 依赖 OD-1 -4. FPC-04（可选清扫，另起提交） -``` +1. 删除类改动**不能只信增量编译** → 每步先 `rm -rf fe-core/target/{classes,test-classes}`。 +2. 全反应堆**必须含测试源**（禁 `-Dmaven.test.skip=true`），且必须 `-Dcheckstyle.skip=true`。 +3. checkstyle `UnusedImports` 是**阻塞门禁** → **只对改动模块**单独跑 `checkstyle:check`。 +4. 🆕 **`-pl` 必须配 `-am`**（否则兄弟模块 `${revision}` 解析不了，报出**像真错的假错**）， + 且 surefire 2.22.2 认 **`-DfailIfNoTests=false`**（不是 `-DfailIfNoSpecifiedTests`）。 + **这两条我这轮都实际踩了** —— `tasklist.md` 里的命令已修正，照抄即可。 --- -## ⚠️ 开工前必读的三条纪律（本仓库已知踩坑） - -1. **删除类改动不能只信增量编译** —— `fe-core/target/classes` 里确实躺着无源文件的陈旧 `.class`。 - 每步先 `rm -rf fe-core/target/{classes,test-classes}`。 -2. **全反应堆必须含测试源**（禁 `-Dmaven.test.skip=true`），且必须 `-Dcheckstyle.skip=true` - （否则 checkstyle 扫 generated-sources 退化成平方级，构建卡死 60+ 分钟）； - checkstyle 改为**只对改动模块**单独跑 `checkstyle:check`。 -3. **`fe-connector-api` 的录制基线要显式跑** —— - `mvn -pl fe-connector/fe-connector-api test`。全反应堆 `test-compile` **不跑 surefire**， - 这是本分支已经红过好几批没人发现的盲区。 - 本任务**预期不需要刷基线**（该模块不依赖 fe-core），**红了就停手，别顺手刷。** +## 🔴 一个必须记住的「别再犯」 ---- - -## 🔎 尚未验证（如实声明，别当成已完成） +调研初判说 `common/` 和 `fe-filesystem-s3-base` 的 +`S3CredentialsProviderType`/`S3CredentialsProviderFactory` 是重复造轮子、可以直接替换 —— +**被对抗验证两轮推翻**。两条活的行为差异（`design.md` §3.3）： +① 发给 hadoop 的凭证串会**多出** `ProfileCredentialsProvider`； +② 模式串接受面**放宽**（空串 / `ENVIRONMENT` / `WEB_IDENTITY_TOKEN_FILE` 从抛异常变成接受）。 +而**全仓没有任何测试钉住那个串** ⇒ 换掉会**绿着上线一个回归**。 -- **没跑过任何 maven 构建** —— `tasklist.md` 里的验证命令是方子，不是结果 -- **没查 apache/doris master** 是否有 `StorageAdapter.getAwsCredentialsProvider()` 的调用者 - （FPC-02 的 rebase 冲突风险是陈述不是实测） -- **没跑 e2e**（需要集群） -- `ExternalCatalog.buildHadoopConfiguration(Map)` 的调用者没枚举 ⇒ FPC-04 明确排除它 +**下次看到「这两个类长得一样，合并掉吧」的念头，先来读 `design.md` §3.3。** --- -## 📎 参考：完整英文调研报告 - -29k 字的原始报告（逐条 `file:line` 证据 + 三份独立设计的分歧点 + 六项对抗验证的两轮判决） -在调研 session 的 scratchpad： -`/tmp/claude-1000/-mnt-disk1-yy-git-wt-catalog-spi/6983e5ef-36cf-4f14-a048-139ffc1c1b51/scratchpad/property-common-metastore-report.md` +## 🔎 尚未验证（如实声明） -⚠️ **scratchpad 是 session 级的，可能已经不在了。** 本空间的 `design.md` 已经把其中**结论性、 -需要长期保留**的部分中文化落盘 —— 以 `design.md` 为准，那份报告只是溯源用。 +- **没跑 e2e**（需要集群）。FPC-03 是纯删除不可达代码 + Gson 回放测试已过，风险低； + 但真正的存储绑定路径（iceberg hadoop `warehouse → fs.defaultFS`）只有单测覆盖。 +- **没查 apache/doris master** 是否有 `getAwsCredentialsProvider()` 调用者 → 这正是 OD-2。 +- `ExternalCatalog.buildHadoopConfiguration(Map)` 的调用者没枚举 ⇒ FPC-04 明确排除它。 diff --git a/plan-doc/fecore-property-cleanup/open-decisions.md b/plan-doc/fecore-property-cleanup/open-decisions.md index 683d99ddb14..7775b83bc67 100644 --- a/plan-doc/fecore-property-cleanup/open-decisions.md +++ b/plan-doc/fecore-property-cleanup/open-decisions.md @@ -74,8 +74,12 @@ FPC-03 要删掉路 B。问题是：**路 A 的 `pluginSupplier` 为 null 时怎 **选 A。** 理由：它是唯一「零行为变更 + 守 fail-loud」的写法，代码量和 B 一样是一行， 而且把一个**今天靠人工审计才知道不可达**的窗口，变成**万一走进去会立刻自曝**。 -> **拍板结果**：（待填） -> **日期**：（待填） +> **拍板结果**：⏳ **按推荐值 A 执行，待用户追认**。2026-07-28 用户指示「直接开始编码」但未就本条表态， +> 遂按文档推荐值 A（`throw new IllegalStateException`）落地于 FPC-03（commit 见 `progress.md`）。 +> **要翻成 B 只需改一行** —— `CatalogProperty.resolveDerivedStorageDefaults()` 的 null 分支改成 +> `return Collections.emptyMap();`，并同步删掉 `CatalogPropertyPluginStorageDerivationTest` +> 的 `unwiredSupplierFailsLoudInsteadOfDerivingNothing` 用例。 +> **日期**：2026-07-28 --- diff --git a/plan-doc/fecore-property-cleanup/progress.md b/plan-doc/fecore-property-cleanup/progress.md index 1a064f44bbd..9eac420834a 100644 --- a/plan-doc/fecore-property-cleanup/progress.md +++ b/plan-doc/fecore-property-cleanup/progress.md @@ -88,3 +88,60 @@ null-supplier 窗口。复核发现**修不干净**：提前之后 lambda 捕获 ### 卡点 **OD-1 待用户拍板**（null-supplier 分支 fail-loud vs fail-silent），阻塞 FPC-03。 + +--- + +## 2026-07-28（二）— FPC-01 + FPC-03 落地 + +**用户指示**：文档空间单独 commit（`938d38c7425`），然后直接开始编码。 + +### FPC-01 —— OD-1 未获表态，按推荐值 A 执行 + +用户说「直接开始编码」但未就 OD-1 表态。按文档推荐值 **A（fail-loud，`throw`）** 落地， +理由是它**精确保留今天的行为**（今天走到这里就是抛），且**翻成 B 只需改一行 + 删一个用例** +——即使追认时被推翻，代价也极小。已在 `open-decisions.md` 标为「⏳ 待用户追认」。 + +### FPC-03 —— 主删除完成 + +**删 5 文件**（`metastore/` 四个 + `ConnectionProperties.java`）**共 473 行**， +`CatalogProperty` 净减 ~45 行。**零 pom / 零连接器业务代码 / 零 fe-filesystem 改动。** + +`resolveDerivedStorageDefaults()` 的 null 分支落地为 +`throw new IllegalStateException("Storage properties were accessed before the connector-derived +storage defaults were wired ...")`。 + +**顺带清掉 3 处悬空注释引用**：`StorageAdapter`（javadoc 散文，非编译依赖）、 +paimon `TcclPinningConnectorContext:49`、`DatasourcePrintableMap:69` +（最后这处是全仓 grep 才揪出来的，初次清单里漏了）。 + +**测试改动**（守 Rule 9）：`CatalogPropertyPluginStorageDerivationTest` +- 类 javadoc + 两处 MUTATION 注释**改钉到仍然存在的变异上**（原来钉的 + 「改回走 `getMetastoreProperties()`」已无法表达） +- **新增**第 4 个用例 `unwiredSupplierFailsLoudInsteadOfDerivingNothing`，钉住 fail-loud 不变量 + +### 验证（全部实跑，非预期） + +| 项 | 结果 | +|---|---| +| 全仓残留 grep（`fe`/`regression-test`/`tools`/`gensrc`） | **0** | +| 全反应堆 `clean test-compile -Dcheckstyle.skip=true`（含测试源） | **BUILD SUCCESS**（2:16） | +| `-pl fe-core checkstyle:check` | **0 violations** ⇒ import 修剪精确 | +| fe-core 定向单测（含 3 个 Gson 回放兼容测试） | **95 run / 0 fail** | +| `-pl fe-connector/fe-connector-api -am test`（录制基线） | **110 run / 0 fail**；`connector-metadata-methods.txt` **未被改动**（与预判一致） | +| 变异验证（`throw` → `return emptyMap()`） | 新用例**变红**，其余三例不受影响；改回**复绿** | + +### 🔴 踩坑记录 + +**坑 4 —— 我自己写进 tasklist 的验证命令有两处是错的，实跑才发现。** +① `mvn -pl fe-core test` **漏了 `-am`** → 兄弟模块的 `${revision}` 解析不了，报的是 +「Could not resolve dependencies / fe-authentication:pom:${revision}」这种**看起来像真错的假错** +（本仓库 `hive-catalog-shade-removal` 的 T-72 早就记过「漏 `-am` = 假错」，我还是踩了）。 +② surefire 2.22.2 认的是 **`-DfailIfNoTests=false`**，我写的 `-DfailIfNoSpecifiedTests=false` 无效， +导致 `-am` 带起来的 `fe-foundation` 因「No tests were executed」直接 FAILURE。 +两条都已修进 `tasklist.md` 的「四条纪律」。 +**通用教训**：**文档里的验证命令在实跑过之前都只是草稿**，别当成已验证的资产传给下一个 session。 + +### 状态 + +- `metastore/` 目录已不存在；`property/` 下只剩 `common` / `constants` / `fileformat`。 +- 下一步：**FPC-02**（删 AWS 死构造臂）——先按 OD-2 grep 一次上游 master。 diff --git a/plan-doc/fecore-property-cleanup/tasklist.md b/plan-doc/fecore-property-cleanup/tasklist.md index 38d5c98acdf..c3356f00ada 100644 --- a/plan-doc/fecore-property-cleanup/tasklist.md +++ b/plan-doc/fecore-property-cleanup/tasklist.md @@ -34,6 +34,8 @@ mvn -f $R/fe/pom.xml -pl fe-core checkstyle:check 2. 全反应堆**必须含测试源**，**禁 `-Dmaven.test.skip=true`**；且必须 `-Dcheckstyle.skip=true` （否则 checkstyle 扫 generated-sources 退化成平方级，构建卡死）。 3. checkstyle 的 `UnusedImports` 是**阻塞门禁**，改为**只对改动模块**单独跑 `checkstyle:check`。 +4. 🆕 **`-pl` 必须配 `-am`**（否则兄弟模块的 `${revision}` 解析不了 → 假错），且 surefire 2.22.2 + 认的是 **`-DfailIfNoTests=false`**（不是 `-DfailIfNoSpecifiedTests`）。2026-07-28 两条都实测踩过。 --- @@ -52,12 +54,16 @@ mvn -f $R/fe/pom.xml -pl fe-core checkstyle:check ## 阶段 1 — 🔴 前置拍板（**不拍板不许开工 FPC-03**） -- [ ] **FPC-01** ⬜ **OD-1 拍板**：删掉 metastore 后，`resolveDerivedStorageDefaults()` 的 null-supplier - 分支要 **fail-loud（`throw`）** 还是 **fail-silent（`return emptyMap()`）**？ - 详见 [`open-decisions.md`](./open-decisions.md) **OD-1**（含推荐值与三个选项的代价）。 - - ⚠️ 这条**改变 FPC-03 的代码**，必须先定。 +- [x] **FPC-01** ⏳ **OD-1：按推荐值 A 执行，待用户追认** + （问题：删掉 metastore 后，`resolveDerivedStorageDefaults()` 的 null-supplier 分支要 + **fail-loud（`throw`）** 还是 **fail-silent（`return emptyMap()`）**？详见 + [`open-decisions.md`](./open-decisions.md) **OD-1**） + - **落地为 A**：`throw new IllegalStateException(...)`，随 FPC-03 一起提交。 + - 配守卫测试 `CatalogPropertyPluginStorageDerivationTest.unwiredSupplierFailsLoudInsteadOfDerivingNothing`， + **已做变异验证**：把 `throw` 改成 `return emptyMap()` → 该用例变红（其余三例不受影响），改回 → 复绿。 + - **要翻成 B 只需改一行 + 删该用例**（OD-1 里写了具体位置）。 - ⚠️ 调研报告原提的缓解方案「把 supplier 安装语句提前」**经复核修不干净** - （lambda 读到的 `connector` 字段仍是旧值/null），已在 OD-1 中列为**不推荐**。 + （lambda 读到的 `connector` 字段仍是旧值/null），已在 OD-1 中列为**不推荐**，未采纳。 --- @@ -91,7 +97,7 @@ mvn -f $R/fe/pom.xml -pl fe-core checkstyle:check grep -rIn 'getAwsCredentialsProvider()\|createV2\|createDefaultV2' $R/fe $R/regression-test --exclude-dir=target rm -rf $R/fe/fe-core/target/classes $R/fe/fe-core/target/test-classes mvn -f $R/fe/pom.xml -T 1C clean test-compile -Dcheckstyle.skip=true - mvn -f $R/fe/pom.xml -pl fe-core test -Dcheckstyle.skip=true -DfailIfNoSpecifiedTests=false \ + mvn -f $R/fe/pom.xml -pl fe-core -am test -Dcheckstyle.skip=true -DfailIfNoTests=false \ -Dtest='AzureGuessRoutingParityTest,S3ThriftAdapterParityTest,CloudObjectStoreAdapterParityTest,LocationPathTest,DefaultConnectorContextBackendStoragePropsTest,DefaultConnectorContextNormalizeUriTest' mvn -f $R/fe/pom.xml -pl fe-core checkstyle:check # 阻塞项：证明 import 修剪精确 ``` @@ -102,7 +108,7 @@ mvn -f $R/fe/pom.xml -pl fe-core checkstyle:check ## 阶段 3 — 主删除（**依赖 FPC-01 拍板**） -- [ ] **FPC-03** ⬜ 退役整个 `metastore/` 集群 + 孤儿 `ConnectionProperties` +- [x] **FPC-03** ✅ 退役整个 `metastore/` 集群 + 孤儿 `ConnectionProperties` （**删 5 文件 473 行 + 从 `CatalogProperty` 挖掉 ~45 行；零 pom / 零连接器 / 零 fe-filesystem 改动**） - **`CatalogProperty.java` 改动**： 1. `resolveDerivedStorageDefaults()`（`:264-272`）→ 只走 supplier； @@ -136,10 +142,10 @@ mvn -f $R/fe/pom.xml -pl fe-core checkstyle:check $R/fe $R/regression-test $R/tools $R/gensrc --exclude-dir=target rm -rf $R/fe/fe-core/target/classes $R/fe/fe-core/target/test-classes mvn -f $R/fe/pom.xml -T 1C clean test-compile -Dcheckstyle.skip=true - mvn -f $R/fe/pom.xml -pl fe-core test -Dcheckstyle.skip=true -DfailIfNoSpecifiedTests=false \ + mvn -f $R/fe/pom.xml -pl fe-core -am test -Dcheckstyle.skip=true -DfailIfNoTests=false \ -Dtest='CatalogPropertyPluginStorageDerivationTest,CatalogPropertyEffectiveRawStoragePropsTest,HmsGsonCompatReplayTest,IcebergGsonCompatReplayTest,PaimonGsonCompatReplayTest,PluginDrivenExternalCatalog*Test' # 🔴 录制基线必须显式跑（全反应堆 test-compile 不跑 surefire —— 本分支已知盲区） - mvn -f $R/fe/pom.xml -pl fe-connector/fe-connector-api test -Dcheckstyle.skip=true + mvn -f $R/fe/pom.xml -pl fe-connector/fe-connector-api -am test -Dcheckstyle.skip=true -DfailIfNoTests=false mvn -f $R/fe/pom.xml -pl fe-core checkstyle:check ``` **预期不需要刷 `connector-metadata-methods.txt`**（fe-connector-api 不依赖 fe-core， --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
