morningman opened a new pull request, #66507:
URL: https://github.com/apache/doris/pull/66507
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Every connector had grown its own way of dealing with catalog properties.
Some kept a
`XxxConnectorProperties` constant class and parsed values at each read site;
some had no property
class at all and inlined the key names; iceberg and paimon had *two* readers
for the same keys — a
typed holder used for CREATE-time validation, and a separate raw-map scan
with its own copy of the
alias arrays used to actually build the catalog. Nothing kept those two in
agreement, so an alias
priority or a blank-value rule could be validated one way and assembled
another.
This PR gives every connector the same four-way split, and makes each key
have exactly one reader.
No new SPI: `ConnectorPluginSurfaceTest` is untouched throughout.
### Where things live now
**A — `<Xxx>CatalogProperties`: what a user writes in `CREATE CATALOG`.**
One class per connector, in the connector module. Fields carry
`@ConnectorProperty` with the alias
list, so a key name and its aliases are declared exactly once, and
`ConnectorPropertiesUtils` binds
them. This is what the connector, the metadata layer and the scan/write
planners read — none of them
touch the raw map for a key this class declares.
The two entry points are deliberately not interchangeable, and this is the
part most worth reviewing:
- `of(Map)` **binds and derives, and never throws.** It runs at CREATE, at
ALTER validation, and on
every connector build — including the lazy rebuild after an FE restart. A
rule placed here that a
live catalog violates does not fail at review time; it fails months later,
as a catalog that stops
coming back after a restart.
- `checkCreateTimeOnlyRules()` carries everything that judges, and only the
provider calls it, from
`validateProperties` — one line in most connectors. Unknown keys are never
rejected: the same map
carries engine keys and storage keys, and `ALTER CATALOG` can only
overwrite a key, never remove
one, so a rejected unknown key could not be repaired.
**B — `<Xxx>Conf`: deployment-level settings.** The keys of the plugin's own
`<name>.conf`, each
falling back to the `fe.conf` key it used to live under. Static accessors
(`driversDir(context)`,
`metastoreClientTimeoutSecond(context)`, …) rather than a bound object,
because these belong to the
deployment and not to any one catalog. Only the connectors that actually
have such settings have one.
**Per-flavor `*MetaStoreProperties` (iceberg / paimon): the keys of one
metastore backend.**
These already existed in `fe-connector-metastore-{iceberg,paimon}` but
described themselves as
"validation only" while the connector re-scanned the same keys to build the
catalog. They are now the
single declaration: they gained the fields the assembly needed and the
getters it reads, and the
factories consume the bound holder instead of `firstNonBlank(props,
ALIASES)`. The alias arrays that
duplicated them are deleted. The metastore modules stay SDK-free — they
expose neutral getters and
maps; engine-SDK option assembly stays in the connector factory.
Splitting by flavor also splits the "annotation count == key count"
invariant: the connector-level
holder declares the flavor-independent keys, each backend holder declares
its own.
**F/G — literals the assembly emits, and mode enums.** Option keys and
values that a connector
*writes* (rather than a user setting them) are private to the class that
writes them —
`IcebergCatalogFactory` for the iceberg SDK dialect, and so on. Cache key
names sit next to the cache
they configure. In several places these had drifted into two spellings of
the same string in two
files; folding them removed those duplicates.
### Responsibilities, end to end
| Layer | Reads | Writes |
|---|---|---|
| `<Xxx>ConnectorProvider` | — | `validateProperties` →
`of(props).checkCreateTimeOnlyRules()` |
| `<Xxx>CatalogProperties` | the raw map, once | typed getters; the derived
flavor |
| `*MetaStoreProperties` | the raw map, once, for its own backend | typed
getters; neutral conf maps |
| `<Xxx>CatalogFactory` | the bound holders (+ raw map only for copy-all /
prefix passthroughs) | the engine-SDK option map |
| `<Xxx>Conf` | `ConnectorContext` conf + environment | — |
| connector / metadata / scan / write | the bound holders | — |
The raw map is still read in three legitimate shapes, each commented where
it happens: copy-all
passthrough into the SDK options, whole-namespace forwarding (`fs.` / `dfs.`
/ `hadoop.`, `paimon.`,
`jdbc.`), and alias sets that span namespaces and so belong to no single
flavor (the S3 region
aliases, the AWS credentials-provider mode).
### Verification
Unit tests only — this is a refactor with no intended behavior change, and
the guard against
unintended change is a set of whole-map snapshot tests added *before* each
rework: paimon 8 cases and
iceberg 20 cases assert the ENTIRE catalog option map, one per flavor and
per emission branch. Both
SDKs silently ignore an option they do not recognize, so a dropped or
misspelled key does not throw —
it produces a catalog that connects with different settings than the
operator asked for. Those tests
stay in the tree afterwards as the permanent guard that the holder and the
assembly agree.
Every touched module, run together at the final commit with
`-Dmaven.build.cache.enabled=false` (the build cache otherwise reports a
stale green):
| module | tests | module | tests |
|---|---|---|---|
| iceberg | 1215 (5 skip) | hudi | 203 |
| paimon | 532 (1 skip) | adbc | 200 |
| hive | 411 | maxcompute | 147 (1 skip) |
| jdbc | 222 | es | 118 |
| connector-spi | 140 | trino | 55 |
| hms shared lib | 107 | metastore-{iceberg,paimon,spi,api} | 77 |
| foundation | 161 | cache framework | 33 |
**3780 tests, 0 failures, 0 errors, 7 skips** — every skip is a pre-existing
live-connectivity test
gated on environment variables, and each was already skipped before this PR.
checkstyle clean;
`ConnectorPluginSurfaceTest` green.
### Behavior changes
Small, and all on inputs that are already degenerate. Full per-connector
tables are in the commit
messages; the classes of change are:
1. **Values are trimmed.** The property binder trims and the old
hand-written scans did not, so a
`uri` written with a trailing space was already *validated* trimmed while
the catalog was *built*
from the untrimmed string. The two now agree. For paimon HMS this removed
an existing internal
inconsistency (HiveConf got the trimmed value, the paimon `Options` got
the raw one).
2. **Blank now means unset**, where old code used `containsKey` /
`getOrDefault`. Affects e.g.
`iceberg.rest.view-enabled = ""` (was false, now its default true) and
`external_catalog.name = ""` (was an empty namespace level, now absent).
3. **Values the FE interprets are now sent downstream interpreted.** This
fixed three real bugs where
the FE parsed a value and then forwarded the unparsed original — the ES
`http_ssl_enabled` payload
to BE, the hive `uri` shorthand, and the trino `connector.name`.
Numeric keys were audited per connector and deliberately left as Strings
wherever the value is
forwarded verbatim to an engine SDK, so that a catalog created with a value
the SDK tolerates keeps
building; the JDBC connection-pool knobs are the one place where the
strict/lenient choice is made
per key, with the reasoning in that commit.
### Release note
None
### Check List (For Author)
- Test
- [x] Unit Test
- [ ] Regression test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- Behavior changed:
- [x] Yes. Whitespace around property values is now trimmed; an
explicitly blank value now reads
as unset rather than as an empty string; and three keys the FE
interprets are now forwarded in
their interpreted form. Details per connector are in the individual
commit messages.
- Does this need documentation?
- [x] No.
--
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]
