Hi All, I think PR [5788] is approaching a mergeable state. Please review it.
[5788] https://github.com/apache/polaris/pull/5788 Thanks, Dmitri. On Fri, Oct 30, 2026 at 7:34 PM Dmitri Bourlatchkov <[email protected]> wrote: > Hi David, > > Thanks for the PR. I left some comments in GH, but overall, I think it is > a nice addition to Polaris. > > Cheers, > Dmitri. > > On Tue, Oct 27, 2026 at 8:19 AM David Chaava via dev < > [email protected]> wrote: > >> Hi everyone, >> >> We’ve addressed and replied to all bot review comments on PR #5788: >> https://github.com/apache/polaris/pull/5788 >> >> We would appreciate another review of the updated implementation, >> including >> feedback on retaining the validator SPI as proposed in this discussion. >> >> The CI workflows are awaiting maintainer approval. Could someone approve >> them so the checks can run? >> >> Thanks, >> David >> >> On Fri, Oct 16, 2026 at 7:10 AM Jean-Baptiste Onofré <[email protected]> >> wrote: >> >> > Hi David >> > >> > Thanks for the update! We will take a look on the PR. >> > >> > Regards >> > JB >> > >> > On Thu, Oct 15, 2026 at 5:01 PM David Chaava via dev < >> > [email protected]> wrote: >> > >> >> Hi Dmitri, JB, and Prithvi, >> >> >> >> We have opened a draft PR implementing the validator approach discussed >> >> here: >> >> >> >> https://github.com/apache/polaris/pull/5788 >> >> >> >> It introduces a CDI-discovered CatalogConfigValidator SPI with a >> BigLake >> >> implementation, keeping PolarisAdminService provider-neutral. >> Validation >> >> runs on creation and update using the effective connection, catalog >> >> properties, and storage configuration. >> >> >> >> The BigLake checks apply only to the canonical endpoint, treating a >> >> trailing slash as equivalent, and cover the required configuration >> >> discussed in this thread. The PR includes validator and >> management-service >> >> tests. >> >> >> >> Outbound header propagation is being addressed separately in #5613 >> >> <https://github.com/apache/polaris/pull/5613>, which is still open. We >> >> plan >> >> to merge the validator after that fix. >> >> >> >> We would appreciate it if you could review this PR so we can move it >> >> forward. >> >> >> >> Thanks, >> >> David >> >> >> >> On Wed, Sep 30, 2026 at 11:06 AM David Chaava <[email protected] >> > >> >> wrote: >> >> >> >> > Hi Dmitri, JB, and Prithvi, >> >> > >> >> > Thank you for the thoughtful feedback. The comments help clarify both >> >> the >> >> > architectural concern with the initial implementation and the >> >> configuration >> >> > gap we intended to address. >> >> > >> >> > Our proposed direction is a CDI-discovered validator SPI with a >> >> > BigLake-specific implementation, keeping `PolarisAdminService` >> >> > provider-neutral. The validator would receive the stored connection >> >> > configuration and the catalog properties and storage configuration >> being >> >> > saved, so the same relevant checks can be applied during creation and >> >> > update. >> >> > >> >> > We’ll scope the BigLake validator to the canonical BigLake Iceberg >> REST >> >> > endpoint, treating a trailing slash as equivalent. Other GCP-backed >> >> Iceberg >> >> > REST catalogs should continue to pass through unchanged. Within that >> >> scope, >> >> > validation would focus on required configuration: tthe remote catalog >> >> name, >> >> > the `x-goog-user-project` value, and GCS settings when >> Polaris-managed >> >> > credential vending requires them. We would avoid broad URL or header >> >> > allowlists. >> >> > >> >> > We agree that outbound custom-header propagation should be addressed >> >> > separately first, with request-level test coverage in >> >> > `IcebergRESTFederatedCatalogFactory`. The Python CLI work will also >> >> remain >> >> > in a separate PR. >> >> > >> >> > Once the header-propagation change is addressed, we’ll prepare the >> >> > validator SPI and BigLake implementation around these boundaries. If >> >> we’ve >> >> > misunderstood any part of the suggested scope, please let us know >> >> before we >> >> > proceed. >> >> > >> >> > Thanks again, >> >> > David >> >> > >> >> > On Sat, Sep 26, 2026 at 7:53 AM Prithvi S < >> [email protected]> >> >> > wrote: >> >> > >> >> >> Hi all, >> >> >> >> >> >> Thanks for writing this up David. I agree with Dmitri and JB. >> >> >> >> >> >> Fail-fast checks on create and update are worth having. The checks >> are >> >> >> just property lookups, and a CDI bean per provider keeps >> >> >> PolarisAdminService free of BigLake details. Calling the beans >> there is >> >> >> fine. BigLake should stay ICEBERG_REST. HIVE and BIGQUERY have their >> >> own >> >> >> connection types because the remote API is different. >> >> >> >> >> >> One thing to fix before we require header.x-goog-user-project: the >> >> >> factory never sends it. Iceberg's RESTSessionCatalog does >> >> >> .withHeaders(RESTUtil.configHeaders(config)). >> >> >> IcebergRESTFederatedCatalogFactory builds the same client and skips >> >> that >> >> >> call, so the key can be on the property map and still never leave >> as a >> >> >> header. The CLI already stores it with --property, which lands on >> >> catalog >> >> >> properties, and the factory merges those in. I would fix the factory >> >> first, >> >> >> on its own, with a test. IcebergRestConnectionConfigInfoDpo already >> >> has an >> >> >> allowlist for this one header. I would leave that list as it is. >> >> Whether >> >> >> BigLake requires the header belongs in the bean. >> >> >> >> >> >> For the bean, match >> >> https://biglake.googleapis.com/iceberg/v1/restcatalog >> >> >> exactly, ignoring a trailing slash. Any other GCP Iceberg REST >> catalog >> >> >> should pass through as it does today. updateCatalog changes >> properties >> >> and >> >> >> storage config and leaves the connection as it was stored, so the >> bean >> >> >> needs the stored connection plus the properties and storage about >> to be >> >> >> saved. The header check has to accept catalog properties too, since >> >> that is >> >> >> where the CLI puts it. Remote catalog name, the quota-project >> header, >> >> and, >> >> >> when credential vending is on, a GCS location and a service account. >> >> Simple >> >> >> presence checks are enough. >> >> >> >> >> >> Python in its own PR, as Dmitri said. Once the factory forwards the >> >> >> header, the CLI can keep storing it where it does today. >> >> >> >> >> >> wdyt? >> >> >> >> >> >> Regards, >> >> >> Prithvi S >> >> >> >> >> >> On Sat, Sep 26, 2026 at 11:10 AM Jean-Baptiste Onofré < >> [email protected] >> >> > >> >> >> wrote: >> >> >> >> >> >>> Hi guys, >> >> >>> >> >> >>> Thanks for bringing this on the dev mailing list David! >> >> >>> >> >> >>> I agree with Dmitri's points. Here's my perspective on the >> questions: >> >> >>> >> >> >>> 1. Validation & abstraction: I think fail-fast is definitely good >> for >> >> >>> user experience, but keeping PolarisAdminService lean and >> >> >>> vendor-neutral is paramount. Dmitri's suggestion to use a >> >> >>> CDI-discovered SPI (e.g. ConnectionConfigValidator or >> >> >>> CatalogConfigValidator) is the right architecture, similar to what >> we >> >> >>> do for Authorizer or S3 vending mechanisms (ok we talk about SPI >> again >> >> >>> in Polaris but that's appropriate here :)). >> >> >>> 2. Ownership, dispatch, and validation scope: since catalog >> creation >> >> >>> and updates are infrequent and validation logic is in-memory, >> running >> >> >>> matching validators during create/update won't impact performance. >> >> >>> However, we should be cautious about over-validation: >> >> >>> a. We should validate structural prerequisites (e.g. ensuring >> >> >>> required credential-vending fields exist when credential vending is >> >> >>> enabled for instance) and obvious misconfigurations. >> >> >>> b. We should avoid overly rigid allowlists or fragile regexs on >> >> >>> URIs/headers. If a validator rejects unknown headers or URL >> formats, >> >> >>> any minor update could break users until a new Polaris release is >> >> >>> available. >> >> >>> 3. Generic ICEBERG_REST vs dedicated connection type: I'm strongly >> in >> >> >>> favor of keeping this under ICEBERG_REST. Introducing a dedicated >> >> >>> ConnectionType for each provider REST catalog would lead to enum >> >> >>> proliferation and defeat the purpose of having a standard >> ICEBERG_REST >> >> >>> federation layer. Dedicated connection types (like HIVE or >> BIGQUERY) >> >> >>> should be reserved for distrinct protocols or SDKs, not variations >> of >> >> >>> standard Iceberg REST endpoints. >> >> >>> 4. Transport & outbound headers (prerequisites): as noted during >> the >> >> >>> initial PR review, before enforcing configuration properties like >> >> >>> header.x-goog-user-project, we need to make sure >> >> >>> IcebergRESTFederatedCatalogFactory actually propagates those >> headers >> >> >>> property to the outbound HTTP client. >> >> >>> >> >> >>> I propose the following next steps: >> >> >>> 1. We introduce a clean CDI-based validator SPI (with BigLake >> >> >>> implemented as one bean) as proposed by Dmitri. >> >> >>> 2. We ensure outbound custom header propagation works property in >> >> >>> IcebergRESTFederatedCatalogFactory. >> >> >>> 3. Follow Dmitri's advice on keeping the Python CLI changes in a >> >> >>> dedicated PR. >> >> >>> >> >> >>> WDYT? >> >> >>> >> >> >>> Regards >> >> >>> JB >> >> >>> >> >> >>> On Sat, Sep 26, 2026 at 2:03 AM Dmitri Bourlatchkov < >> [email protected] >> >> > >> >> >>> wrote: >> >> >>> > >> >> >>> > Hi David, >> >> >>> > >> >> >>> > Thanks for stating this thread! >> >> >>> > >> >> >>> > > 1. Is provider-specific configuration validation desirable for >> >> >>> generic >> >> >>> > > `ICEBERG_REST` federation? >> >> >>> > >> >> >>> > From POV, having validation can help users detect mistakes early. >> >> It is >> >> >>> > nice to have. >> >> >>> > >> >> >>> > In this particular case, the validation is based purely on >> pattern >> >> >>> matching >> >> >>> > against existing catalog properties. It does not involve I/O or >> >> >>> external >> >> >>> > dependencies. >> >> >>> > >> >> >>> > I believe this kind of validation should be ok to keep in the >> main >> >> >>> codebase >> >> >>> > (runtime/service). I wonder what other people think about this. >> >> >>> > >> >> >>> > Side note: Please consider making Python changes in separate PRs >> as >> >> >>> they >> >> >>> > usually need different reviewers ;) >> >> >>> > >> >> >>> > > 3. What ownership and dispatch boundary would keep the shared >> >> admin >> >> >>> > > workflow provider-neutral and avoid running provider-specific >> >> logic >> >> >>> for >> >> >>> > > every catalog operation? >> >> >>> > >> >> >>> > Regarding abstraction, I would suggest using CDI to find multiple >> >> >>> > implementations of a (new) common validation interface. Admin >> >> Service >> >> >>> code >> >> >>> > will stay lean and only call the validation interface. That call >> >> will >> >> >>> be >> >> >>> > dispatched to every matching CDI bean, each running logic >> specific >> >> to >> >> >>> a use >> >> >>> > case (e.g. BigLake). >> >> >>> > >> >> >>> > Since validation code is involved only on updates (unfrequently), >> >> and >> >> >>> > valation logic likely runs in-memory (fast), I do not think we >> need >> >> to >> >> >>> > worry about calling every validators on every change. >> >> >>> > >> >> >>> > The PR [5513] shows some nice (recent) examples of leveraging CDI >> >> for >> >> >>> > pluggable components. >> >> >>> > >> >> >>> > [5513] https://github.com/apache/polaris/pull/5513 >> >> >>> > >> >> >>> > Cheers, >> >> >>> > Dmitri. >> >> >>> > >> >> >>> > On Wed, Sep 23, 2026 at 12:26 AM David Chaava via dev < >> >> >>> > [email protected]> wrote: >> >> >>> > >> >> >>> > > Hi Polaris community, >> >> >>> > > >> >> >>> > > >> >> >>> > > We would like guidance on whether Polaris should support >> >> >>> provider-specific >> >> >>> > > configuration validation for external Iceberg REST catalogs, >> and, >> >> if >> >> >>> so, >> >> >>> > > what the appropriate extension model should be. >> >> >>> > > >> >> >>> > > >> >> >>> > > Context: >> >> >>> > > - Polaris supports the generic `ICEBERG_REST` connection type. >> >> >>> > > - The initial BigLake implementation added provider-specific >> >> >>> validation in >> >> >>> > > the shared `PolarisAdminService` create/update path. >> >> >>> > > - Review feedback on PR #5196 raised concerns about putting >> >> >>> > > provider-specific logic in the main admin workflow, >> introducing a >> >> >>> validator >> >> >>> > > without a shared abstraction, and coupling provider >> configuration >> >> >>> rules to >> >> >>> > > Polaris releases. >> >> >>> > > >> >> >>> > > >> >> >>> > > We have closed that implementation and would like to agree on >> the >> >> >>> design >> >> >>> > > before proposing a replacement. >> >> >>> > > >> >> >>> > > >> >> >>> > > Questions: >> >> >>> > > 1. Is provider-specific configuration validation desirable for >> >> >>> generic >> >> >>> > > `ICEBERG_REST` federation? >> >> >>> > > 2. If so, should Polaris first define a shared abstraction or >> >> >>> extension >> >> >>> > > model for such validation? >> >> >>> > > 3. What ownership and dispatch boundary would keep the shared >> >> admin >> >> >>> > > workflow provider-neutral and avoid running provider-specific >> >> logic >> >> >>> for >> >> >>> > > every catalog operation? >> >> >>> > > 4. Under what conditions, if any, would a separate connection >> >> type be >> >> >>> > > preferable to extending the generic `ICEBERG_REST` flow? >> >> >>> > > >> >> >>> > > >> >> >>> > > Links: >> >> >>> > > - Tracking issue: >> https://github.com/apache/polaris/issues/5195 >> >> >>> > > - Closed initial PR: >> https://github.com/apache/polaris/pull/5196 >> >> >>> > > - Architecture feedback: >> >> >>> > > >> >> https://github.com/apache/polaris/pull/5196#discussion_r3846267047 >> >> >>> > > >> >> >>> > > >> >> >>> > > Thanks, >> >> >>> > > David >> >> >>> > > >> >> >>> >> >> >> >> >> >> > >> >
