github-actions[bot] commented on code in PR #68585:
URL: https://github.com/apache/doris/pull/68585#discussion_r4134824627
##########
fe/fe-filesystem/fe-filesystem-hdfs-base/src/main/java/org/apache/doris/filesystem/hdfs/properties/HdfsPropertiesUtils.java:
##########
@@ -218,10 +218,14 @@ public static void checkHaConfig(Map<String, String>
hdfsProperties) {
// No nameservice configured => HA is not enabled, nothing to
validate
return;
}
- for (String dfsservice : splitAndTrim(dfsNameservices)) {
- if (dfsservice.isEmpty()) {
- continue;
- }
+ if (Arrays.stream(dfsNameservices.split(",",
-1)).anyMatch(StringUtils::isBlank)) {
Review Comment:
[P1] Keep previously stored HA catalogs readable after replay. A catalog
with complete `ns1` HA settings and `dfs.nameservices=ns1,` passed the old
`splitAndTrim` check; [Hadoop's
parser](https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java)
also drops the empty trailing token. Replay skips CREATE/ALTER validation, but
runtime HDFS binding calls this new guard and now throws, making that existing
catalog unreadable after upgrade. If submitted empty entries must be rejected,
check them at the DDL boundary while retaining tolerant binding for stored
values.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -253,11 +256,27 @@ public boolean validatePropertiesBeforeUpdate(
} catch (IllegalArgumentException e) {
throw new DdlException(e.getMessage(), e);
}
+ validateStorageProperties(candidateProperty);
checkDriverUrlsAgainstOperatorGate(candidate, updatedProperties);
ExternalFunctionRules.check(candidateProperty.getOrDefault("function_rules",
null));
return true;
}
+ /**
+ * Bind Hive catalog storage properties during DDL validation so HDFS
configuration errors are
+ * reported by CREATE/ALTER instead of being deferred until the first
table access.
+ */
+ private void validateStorageProperties(CatalogProperty property) throws
DdlException {
+ if (!"hms".equalsIgnoreCase(getType())) {
+ return;
+ }
+ try {
+
FileSystemFactory.bindAllStorageProperties(property.getProperties());
Review Comment:
[P2] Bind Hadoop resources before taking the catalog write lock. CREATE and
ALTER reach this call while holding `CatalogMgr`'s global write lock, and an
HMS catalog with `hadoop.config.resources` makes HDFS binding open and parse
each XML file under that lock. A large config file or slow mounted directory
stalls unrelated catalog reads and DDL, with no size or latency bound in this
path. Validate a detached snapshot outside the lock, then recheck catalog state
before publishing it.
##########
fe/fe-core/src/main/java/org/apache/doris/fs/FileSystemFactory.java:
##########
@@ -137,14 +137,14 @@ public static
List<org.apache.doris.filesystem.properties.StorageProperties> bin
System.setProperty("doris.hadoop.config.dir",
Config.hadoop_config_dir);
FileSystemPluginManager mgr = pluginManager;
if (mgr != null) {
- return new ArrayList<>(mgr.bindAll(properties));
+ return new
ArrayList<>(mgr.bindAll(StorageAdapter.withHadoopConfigDir(properties)));
Review Comment:
[P2] Preserve absolute Hadoop resource paths at this binding entry point.
For `hadoop.config.resources=/etc/hadoop/hdfs-site.xml`, the default nonblank
`Config.hadoop_config_dir` makes `withHadoopConfigDir` inject
`_HADOOP_CONFIG_DIR_`; `HdfsConfigFileLoader.load` then concatenates both paths
and reports the existing file missing. Before this change an HMS catalog with
`test_connection=false` could be created and used for metadata listing with
that absolute resource; the new DDL bind rejects it. Resolve absolute resources
as-is and cover this factory path in a 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]
