wangzhigang1999 commented on code in PR #7451: URL: https://github.com/apache/kyuubi/pull/7451#discussion_r3524592995
########## kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigAudience.scala: ########## @@ -0,0 +1,73 @@ +/* + * 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.kyuubi.config + +import org.apache.kyuubi.engine.EngineType + +object ConfigAudience extends Enumeration { + type ConfigAudience = Value + + val SERVER, SPARK, FLINK, HIVE, TRINO, JDBC, DATA_AGENT, ALL_ENGINES, ANY = Value + + private val ENGINE_VALUES: Set[Value] = Set(SPARK, FLINK, HIVE, TRINO, JDBC, DATA_AGENT) + + def fromEngineType(engineType: EngineType.EngineType): ConfigAudience = engineType match { + case EngineType.SPARK_SQL => SPARK + case EngineType.FLINK_SQL => FLINK + case EngineType.HIVE_SQL => HIVE + case EngineType.TRINO => TRINO + case EngineType.JDBC => JDBC + case EngineType.DATA_AGENT => DATA_AGENT + case _ => throw new IllegalArgumentException(s"Unsupported engine type: $engineType") + } + + def isAudienceMatch( + configAudience: Set[ConfigAudience], + target: ConfigAudience): Boolean = { + configAudience.contains(ANY) || + (configAudience.contains(ALL_ENGINES) && ENGINE_VALUES.contains(target)) || + configAudience.contains(target) + } + + def inferAudience(key: String): Set[ConfigAudience] = { + if (key.startsWith("kyuubi.server.") || key.startsWith("kyuubi.frontend.") || Review Comment: This inference strips config that the engine actually reads at runtime. The engine runs its own thrift frontend (`SparkTBinaryFrontendService extends TBinaryFrontendService`, built on the shared kyuubi-common code) and reads at least: - `kyuubi.frontend.connection.url.use.hostname` (`TFrontendService#connectionUrl` — decides whether the engine registers to service discovery by hostname or IP; critical on K8s) - `kyuubi.frontend.thrift.min.worker.threads` / `.max.worker.threads` / `.worker.keepalive.time` (`TBinaryFrontendService`) None of these are `serverOnly` on master (they're `fallbackConf` entries), so today they're passed to the engine on the command line via `conf.getAll`. After this change they're inferred as `SERVER` and filtered out, and the engine silently falls back to defaults. Their fallback keys are also `kyuubi.frontend.*` → also inferred `SERVER`, so the fallback materialization in `getEngineConf` can't recover them either. Could we give these entries an explicit `.audience(...)` that includes the engines (the manual-override escape hatch this PR introduces), plus a test asserting they survive in `getEngineConf` for each engine type? Since these are registered entries, inference keeps applying to them unless they're annotated explicitly. > Note: `kyuubi.frontend.advertised.host` is also read by `connectionUrl`, but it's already `serverOnly` on master (not forwarded today), so it's not a regression and needs no change. ########## kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiReservedKeys.scala: ########## Review Comment: `KyuubiReservedKeys.reservedKeys` is hand-maintained — a future reserved key forgotten here that matches a server prefix would be silently dropped from engine config. A reflection test asserting all constants are in the set would lock it down. ########## kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala: ########## @@ -214,11 +205,47 @@ case class KyuubiConf(loadSysDefault: Boolean = true) extends Logging { getAllWithPrefix(s"$USER_DEFAULTS_CONF_QUOTE${user}$USER_DEFAULTS_CONF_QUOTE", "")) { cloned.set(k, v) } - serverOnlyConfEntries.foreach(cloned.unset) - serverOnlyPrefixConfigKeys.foreach(cloned.unset) cloned } + def getEngineConf(engineType: EngineType.EngineType): Map[String, String] = { + val targetAudience = ConfigAudience.fromEngineType(engineType) + val extraServerPrefixes = get(KyuubiConf.SERVER_ONLY_PREFIXES) + + def resolveAudience(key: String): Set[ConfigAudience.Value] = { + if (KyuubiReservedKeys.reservedKeys.contains(key)) { + Set(ConfigAudience.ANY) + } else { + val entry = KyuubiConf.getConfigEntryOrAlternative(key) + if (entry != null && entry.audience.isDefined) { + entry.audience.get + } else if (extraServerPrefixes.exists(prefix => key.startsWith(prefix))) { Review Comment: Behavior is fine, but the doc is now stale: `SERVER_ONLY_PREFIXES` still says "it only take affects for the configs that is not defined as a Kyuubi ConfigEntry," while this branch now also applies to registered entries without an explicit audience (master excluded registered entries via `getConfigEntry(key) == null`). Please update that doc line. ########## kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigAudience.scala: ########## @@ -0,0 +1,73 @@ +/* + * 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.kyuubi.config + +import org.apache.kyuubi.engine.EngineType + +object ConfigAudience extends Enumeration { + type ConfigAudience = Value + + val SERVER, SPARK, FLINK, HIVE, TRINO, JDBC, DATA_AGENT, ALL_ENGINES, ANY = Value + + private val ENGINE_VALUES: Set[Value] = Set(SPARK, FLINK, HIVE, TRINO, JDBC, DATA_AGENT) + + def fromEngineType(engineType: EngineType.EngineType): ConfigAudience = engineType match { + case EngineType.SPARK_SQL => SPARK + case EngineType.FLINK_SQL => FLINK + case EngineType.HIVE_SQL => HIVE + case EngineType.TRINO => TRINO + case EngineType.JDBC => JDBC + case EngineType.DATA_AGENT => DATA_AGENT + case _ => throw new IllegalArgumentException(s"Unsupported engine type: $engineType") + } + + def isAudienceMatch( + configAudience: Set[ConfigAudience], + target: ConfigAudience): Boolean = { + configAudience.contains(ANY) || + (configAudience.contains(ALL_ENGINES) && ENGINE_VALUES.contains(target)) || + configAudience.contains(target) + } + + def inferAudience(key: String): Set[ConfigAudience] = { + if (key.startsWith("kyuubi.server.") || key.startsWith("kyuubi.frontend.") || + key.startsWith("kyuubi.backend.server.")) { + Set(SERVER) + } else if (key.startsWith("kyuubi.engine.spark.") || + key.startsWith("kyuubi.session.engine.spark.") || key.startsWith("spark.")) { + Set(SPARK) + } else if (key.startsWith("kyuubi.engine.flink.") || + key.startsWith("kyuubi.session.engine.flink.") || key.startsWith("flink.")) { + Set(FLINK) + } else if (key.startsWith("kyuubi.engine.hive.") || + key.startsWith("kyuubi.session.engine.hive.") || key.startsWith("hive.")) { + Set(HIVE) + } else if (key.startsWith("kyuubi.engine.trino.") || + key.startsWith("kyuubi.session.engine.trino.") || key.startsWith("trino.")) { + Set(TRINO) + } else if (key.startsWith("kyuubi.engine.jdbc.") || + key.startsWith("kyuubi.session.engine.jdbc.")) { + Set(JDBC) + } else if (key.startsWith("kyuubi.engine.data.agent.") || + key.startsWith("kyuubi.session.engine.data-agent.")) { Review Comment: This branch matches no key and hard-codes a separator inconsistent with the rest of data-agent. Real keys are dot-separated (`kyuubi.engine.data.agent.*`), but this branch — and the `ConfigAudienceSuite` assertion on `kyuubi.session.engine.data-agent.main.resource` — uses a hyphen for a key that has no `buildConf` anywhere. Drop the branch and its test; add it back with the real (dot) naming if a session-level data-agent key lands. -- 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]
