github-actions[bot] commented on code in PR #66890:
URL: https://github.com/apache/doris/pull/66890#discussion_r3804998145
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnector.java:
##########
@@ -489,14 +512,78 @@ private Catalog createCatalogFromContext(CatalogContext
catalogContext, String f
ClassLoader previous = Thread.currentThread().getContextClassLoader();
try {
Thread.currentThread().setContextClassLoader(getClass().getClassLoader());
- return context.executeAuthenticated(() ->
CatalogFactory.createCatalog(catalogContext));
+ return context.executeAuthenticated(() -> {
+ Catalog catalog = PaimonCatalogProperties.HMS.equals(flavor)
+ ? createHmsCatalog(catalogContext, hmsAuth,
catalogProps.getRaw(),
+ storageHadoopConfig)
+ : CatalogFactory.createCatalog(catalogContext);
+ return catalog;
+ });
} catch (Exception e) {
throw new RuntimeException(failureMessage + " (flavor=" + flavor +
"): " + e.getMessage(), e);
} finally {
Thread.currentThread().setContextClassLoader(previous);
}
}
+ static Catalog createHmsCatalog(CatalogContext catalogContext,
HadoopAuthenticator hmsAuth,
+ Map<String, String> properties, Map<String, String>
storageHadoopConfig) {
+ HiveConf hiveConf = HiveCatalog.createHiveConf(catalogContext);
+ Options options = catalogContext.options();
+ String warehouse = options.get(CatalogOptions.WAREHOUSE);
+ if (warehouse == null) {
+ warehouse =
hiveConf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname,
+ HiveConf.ConfVars.METASTOREWAREHOUSE.defaultStrVal);
+ }
+ Path warehousePath = new Path(warehouse);
+ Path fileIoPath = warehousePath.toUri().getScheme() == null
+ ? new Path(FileSystem.getDefaultUri(hiveConf)) : warehousePath;
+ try {
+ FileIO fileIO = FileIO.get(fileIoPath, catalogContext);
+ // Paimon checks or creates the warehouse eagerly; it must retain
the outer storage identity.
+ fileIO.checkOrMkdirs(warehousePath);
+ String clientClass =
options.get(HiveCatalogOptions.METASTORE_CLIENT_CLASS);
+ Catalog catalog = hmsAuth == null
+ ? new HiveCatalog(fileIO, hiveConf, clientClass, options,
warehousePath.toUri().toString())
+ : hmsAuth.doAs(() -> new HiveCatalog(
+ fileIO, hiveConf, clientClass, options,
warehousePath.toUri().toString()));
+ catalog = PaimonHmsClientPool.install(catalog, hmsAuth);
Review Comment:
**[P1] Compute Paimon format-table owners under the HMS identity**
Installing the boundary only on `HiveCatalog.clients` is too late for
`type=format-table`. In Paimon 1.3.1, `createFormatTable` evaluates
`createHiveFormatTable(...)` before `clients.execute(...)`, and `newHmsTable`
captures `UserGroupInformation.getCurrentUser()` at that earlier point, under
Doris's outer storage/process identity. The subsequent create RPC is
authenticated correctly, but the persisted owner is wrong. Normal Paimon tables
are safe because `createHiveTable(...)` is evaluated inside the client-pool
action. Please scope the format-table owner computation to the HMS UGI without
moving `initialTableLocation`/FileIO work, and add a mixed-identity real
format-table test that checks both owner and storage UGI.
##########
fe/fe-connector/fe-connector-hms/src/main/java/org/apache/doris/connector/hms/HmsConfHelper.java:
##########
@@ -58,21 +82,95 @@ public static HiveConf createHiveConf(Map<String, String>
properties) {
// (fixes SecurityUtil.<clinit>) but cannot fix this conf-cached CL.
Pinning here keeps the whole
// hive-metastore class graph in one loader.
hiveConf.setClassLoader(HmsConfHelper.class.getClassLoader());
+ addConfResources(hiveConf, confResources);
for (Map.Entry<String, String> entry : properties.entrySet()) {
+ // A blank username was ignored by the legacy copy-if-present
path; preserving a resource value (or
+ // the "hadoop" default) avoids createRemoteUser("") failing
before the first HMS RPC.
+ if ("hadoop.username".equals(entry.getKey()) &&
isBlank(entry.getValue())) {
+ continue;
+ }
hiveConf.set(entry.getKey(), entry.getValue());
}
// A kerberized HMS requires SASL transport on the metastore Thrift
connection. The legacy fe-core
// HMSBaseProperties.initHadoopAuthenticator auto-enabled
hive.metastore.sasl.enabled whenever the
// metastore/hadoop auth was kerberos; preserve that here so a catalog
that only declares kerberos auth
// (without an explicit hive.metastore.sasl.enabled) still negotiates
SASL, instead of opening a plain
// TSocket that a kerberized metastore drops with TTransportException.
- if
("kerberos".equalsIgnoreCase(properties.get("hadoop.security.authentication"))
- ||
"kerberos".equalsIgnoreCase(properties.get("hive.metastore.authentication.type")))
{
+ String hmsAuthType =
properties.get("hive.metastore.authentication.type");
+ boolean explicitSimple = "simple".equalsIgnoreCase(hmsAuthType);
+ if (explicitSimple) {
+ // The explicit HMS mode is authoritative even when a base
hive-site.xml enables SASL.
+ hiveConf.set("hive.metastore.sasl.enabled", "false");
+ } else if ("kerberos".equalsIgnoreCase(hmsAuthType)
+ || (!explicitSimple
+ &&
"kerberos".equalsIgnoreCase(properties.get("hadoop.security.authentication"))))
{
hiveConf.set("hive.metastore.sasl.enabled", "true");
}
return hiveConf;
}
+ /**
+ * Creates the lightweight Hadoop configuration used only for UGI
resolution.
+ */
+ public static Configuration createHadoopConfWithResources(String
confResources,
+ Map<String, String> properties) {
+ Configuration conf = new Configuration();
+ conf.setClassLoader(HmsConfHelper.class.getClassLoader());
+ addConfResources(conf, confResources);
+ for (Map.Entry<String, String> entry : properties.entrySet()) {
+ if ("hadoop.username".equals(entry.getKey()) &&
isBlank(entry.getValue())) {
+ continue;
+ }
+ conf.set(entry.getKey(), entry.getValue());
+ }
+ return conf;
+ }
+
+ /**
+ * Preserves connector-agnostic passthrough keys while applying canonical
HMS overrides last.
+ */
+ public static Map<String, String> mergeCatalogProperties(Map<String,
String> raw,
+ Map<String, String> overrides) {
+ Map<String, String> merged = new LinkedHashMap<>(raw);
+ // Canonical parsing deliberately omits a blank username; remove the
raw value so it cannot reappear
+ // merely because the HMS client also preserves unrelated custom
configuration keys.
+ if (isBlank(merged.get("hadoop.username"))) {
+ merged.remove("hadoop.username");
+ }
+ merged.putAll(overrides);
+ return merged;
+ }
+
+ private static void addConfResources(Configuration conf, String
confResources) {
+ if (isBlank(confResources)) {
+ return;
+ }
+ String baseDir = resolveHadoopConfigDir();
+ for (String resource : confResources.split(",")) {
+ File file = new File(baseDir, resource.trim());
+ if (!file.isFile()) {
+ throw new IllegalArgumentException("Config resource file does
not exist: " + file);
+ }
+ conf.addResource(new Path(file.toURI()));
+ }
+ }
+
+ private static String resolveHadoopConfigDir() {
+ String configured = System.getProperty("doris.hadoop.config.dir");
Review Comment:
**[P1] Initialize the custom Hadoop config directory before loading
resources**
`resolveHadoopConfigDir()` can run before the engine publishes
`Config.hadoop_config_dir`: the only production writer of
`doris.hadoop.config.dir` is the lazy
`FileSystemFactory.bindAllStorageProperties` path. Hive builds its HMS client
without touching storage, and Hudi evaluates `getOrCreateClient()` before
`storageHadoopConfig(context)`, so on a clean FE with a non-default
`hadoop_config_dir` the first `hive.conf.resources=hive-site.xml` access looks
under `$DORIS_HOME/plugins/hadoop_conf` and fails; it starts working only after
an unrelated storage/Iceberg/Paimon path initializes the bridge. The new tests
pre-set the system property and miss this ordering. Please initialize/pass the
configured directory deterministically before resource lookup and cover
first-access Hive and Hudi with the property initially unset.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java:
##########
@@ -969,8 +976,43 @@ private Catalog createCatalog() {
LOG.info("Creating Iceberg catalog '{}' flavor='{}' impl='{}'",
catalogName, flavor,
catalogOptions.get(CatalogProperties.CATALOG_IMPL));
- return buildCatalogAuthenticated(flavor,
- () -> CatalogUtil.buildIcebergCatalog(catalogName,
catalogOptions, conf));
+ return buildCatalogAuthenticated(flavor, () -> {
+ if (!IcebergCatalogProperties.TYPE_HMS.equals(flavor)) {
+ return CatalogUtil.buildIcebergCatalog(catalogName,
catalogOptions, conf);
+ }
+ HadoopAuthenticator hmsAuth = buildHmsAuthenticator(properties,
storageHadoopConfig);
+ applyHmsTableOwner(catalogOptions, hmsAuth);
+ Catalog catalog = CatalogUtil.buildIcebergCatalog(catalogName,
catalogOptions, conf);
+ return IcebergHmsClientPool.install(catalog, hmsAuth);
+ });
+ }
+
+ static String appendHmsCacheKeys(String existing) {
+ String keys = appendCacheKey(existing, "conf:hadoop.username");
+ keys = appendCacheKey(keys, "conf:hive.metastore.client.principal");
+ return appendCacheKey(keys, "conf:hadoop.kerberos.principal");
Review Comment:
**[P1] Include the HMS transport settings in static pool keys**
These keys still omit both `hive.metastore.kerberos.principal` and the
effective `hive.metastore.sasl.enabled`, even though the first inner
`HiveClientPool` captures those values in its HiveConf. Iceberg's and Paimon's
caches are JVM-static, so a same-URI `ALTER CATALOG` that corrects or rotates
only the service principal reuses the old target. Likewise, explicit SIMPLE
over Kerberos storage and the storage-Kerberos HMS fallback can have identical
current key values while requiring plain versus SASL transports, so whichever
pool is created first wins. This is separate from the client-UGI and
case-suffix fixes. Please append the exact canonical
`conf:hive.metastore.kerberos.principal` and `conf:hive.metastore.sasl.enabled`
keys here and in Paimon, and test same-URI rebuilds for both target-principal
and false-vs-true SASL changes.
--
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]
