nevzheng commented on issue #11943: URL: https://github.com/apache/gravitino/issues/11943#issuecomment-4913383456
**Proposal** Two concrete changes, plus an honest note on priority. **1) Clarify `warehouse` in the OpenAPI spec.** Today [`CatalogCreateRequest.properties`](https://github.com/apache/gravitino/blob/27ad2cac7a3b74cc689b7850a4524d17d102e1cf/docs/open-api/catalogs.yaml#L378-L384) is an untyped `map<string,string>` with no documented keys, so `warehouse`'s per-backend meaning lives only in prose/code. The property bag **stays a free-form map** — we're just documenting *one of its possible keys* while leaving everything else open (`additionalProperties` is untouched, so it's non-breaking and the plugin model is unaffected). Concretely, we add the JSON Schema `properties` keyword *inside* the map object, alongside `additionalProperties`: ```yaml properties: # the catalog property bag (unchanged: still an open map) type: object description: A map of properties for the catalog nullable: true default: { } properties: # document ONE known key; the map stays open warehouse: type: string description: > Behavior depends on catalog-backend (lakehouse-iceberg): • hive / jdbc: a STORAGE LOCATION (e.g. s3a://bucket/wh, hdfs://ns/path). Required. • rest: a CATALOG SELECTOR forwarded to the Iceberg REST server as ?warehouse= (a catalog name for Gravitino's IRC; an opaque id/ARN for some servers, e.g. S3 Tables). Optional; omit for the default catalog. NOT a storage location. additionalProperties: # unchanged: every other key is still a free string type: string ``` *(Sketch, not a final shape — open to modeling it as a `oneOf`/discriminator keyed on `catalog-backend` instead, or docs-only. Flagging that the OpenAPI here is doc-only; enforcement lives in `IcebergCatalogPropertiesMetadata`, which already types these keys. Feedback on the modeling approach welcome.)* This makes the contract match what the code already declares and removes the surprise (POLA). **2) Validate `warehouse` at create for *all* backends — no delayed failures.** The core bug is that create returns `200` and the mistake only surfaces on the first operation. Gravitino already has the mechanism (`IcebergCatalogOperations.testConnection` → `listNamespace`, which forces the resolve); the fix is to run that validation as part of create so any warehouse problem fails **at create, with an actionable hint** — not later. Crucially, validation must *resolve*, not pattern-match the value: a URI/ARN `warehouse` is legitimately correct for S3 Tables, so we let the backend be the oracle rather than rejecting on shape. **Compact flow (proposed create-time validation):** ```mermaid flowchart TD C["POST /catalogs (create)"] --> B{catalog-backend?} B -->|hive / jdbc| L{warehouse present<br/>& reachable as a location?} L -->|yes| OK1["✅ create"] L -->|no| E1["❌ fail fast:<br/>'warehouse required; must be a storage location'"] B -->|rest| V["eager resolve: GET /v1/config?warehouse=…"] V --> R{resolves?} R -->|yes / omitted → default| OK2["✅ create"] R -->|no| E2["❌ fail fast:<br/>'warehouse did not resolve as a catalog selector —<br/>remove it for the default catalog, or use a catalog name'"] classDef good fill:#e6ffed,stroke:#2ea043,color:#003300; classDef bad fill:#ffebe9,stroke:#cf222e,color:#330000; class OK1,OK2 good; class E1,E2 bad; ``` Same principle for every backend: **attempt to use `warehouse` at create, so it fails now instead of later.** **On priority — yes, this is an edge case.** The workaround is trivial (omit or rename the property), so this is low-priority, not urgent. But it's the kind of paper cut that's cheap to prevent and disproportionately confusing when hit — silent accept + a cryptic delayed `NoSuchWarehouseException`, on the REST surface, in a demo/eval setting. Filing it as *the right small fix*, not a fire. Even part (1) alone (docs) is worth landing on its own. **References** - Spec today: [`CatalogCreateRequest.properties` — untyped string map](https://github.com/apache/gravitino/blob/27ad2cac7a3b74cc689b7850a4524d17d102e1cf/docs/open-api/catalogs.yaml#L378-L384) - Why the surprise is the bug: [Principle of Least Astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment) - Ideal long-term model (typed per backend): [OpenAPI polymorphism / discriminator](https://swagger.io/docs/specification/v3_0/data-models/inheritance-and-polymorphism/) - Why validation must *resolve*, not reject on shape (URI/ARN `warehouse` is valid here): [AWS S3 Tables Iceberg REST — warehouse = bucket ARN](https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-tables-integrating-open-source.html) - Related: #5756 (made `warehouse` optional for REST), #9810 (hide `warehouse` in Web UI for REST), PR #9811 (documented the distinction in the catalog docs) -- 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]
