Hi all, Thanks JB, Dmitri, Yufei and Nandor for the reviews and the discussion.
To close the thread: PR #5909 is merged. Invalid connection configurations of an EXTERNAL catalog now return 400 instead of 500, and the Management API spec documents the 400 response of createCatalog. Noted on the review period for API-level changes; I will keep that in mind for future proposals. Best regards, Youngrae Kim 2026년 11월 6일 (금) 오전 9:03, Dmitri Bourlatchkov via dev < [email protected]>님이 작성: > Hi Yufei, > > No worries, it's not a big deal and it is not about reverting. The change > is good and approved by many (including myself). > > This is about giving other reviewers a chance to express their opinions. > > Cheers, > Dmitri. > > On Thu, Nov 5, 2026 at 6:49 PM Yufei Gu <[email protected]> wrote: > > > Dmitri, sorry I missed the proposal to merge it on Nov 6. Let me know > > if reverting it is needed. > > > > Yufei > > > > On Thu, Nov 5, 2026 at 2:39 PM Dmitri Bourlatchkov via dev > > <[email protected]> wrote: > > > > > > Hi Yufei, > > > > > > I value your approval of 5909, but I guess you might have not noticed > the > > > proposal for an extended review period in my previous email, before > > > you merged it. > > > > > > Cheers, > > > Dmitri. > > > > > > On Thu, Nov 5, 2026 at 5:17 PM Yufei Gu <[email protected]> wrote: > > > > > > > Good catch, Youngrae! I consider this to be a bug fix, so I do not > > > > think we need to worry about compatibility issues. > > > > > > > > On Thu, Nov 5, 2026 at 8:39 AM Dmitri Bourlatchkov via dev > > > > <[email protected]> wrote: > > > > > > > > > > Hi JB, Youngrae, > > > > > > > > > > I totally agree that this change is correct and it is not an > backward > > > > > compatibility issue. > > > > > > > > > > Nice catch on adding the 400 response to the API spec! > > > > > > > > > > I approved PR [5909] in GH. > > > > > > > > > > As usual for API-level changes, let's give it some more time for > > reviews. > > > > > Merging late on Nov 6 seems reasonable to me, if there are no > > > > > further comments. > > > > > > > > > > Cheers, > > > > > Dmitri. > > > > > > > > > > [5909] https://github.com/apache/polaris/pull/5909 > > > > > > > > > > On Thu, Nov 5, 2026 at 4:23 AM youngrae kim < > [email protected]> > > > > wrote: > > > > > > > > > > > Hi JB, > > > > > > > > > > > > Thanks for the review and the +1. > > > > > > > > > > > > Good point on the 400 response. > > > > > > I have added it to createCatalog in the Management API spec in > the > > > > latest > > > > > > commit of the PR: the response is declared with the description > > > > > > "The request is invalid, for example an unsupported or incomplete > > > > storage > > > > > > or connection configuration". > > > > > > No model or generated interface changes, the generated code > > compiles as > > > > > > before. > > > > > > > > > > > > If there are no further objections I will leave the PR for the > > > > committers > > > > > > to merge. > > > > > > > > > > > > Best regards, > > > > > > Youngrae Kim > > > > > > > > > > > > 2026년 11월 5일 (목) 오후 5:31, Jean-Baptiste Onofré <[email protected] > >님이 > > 작성: > > > > > > > > > > > > > Hi Youngrae, > > > > > > > > > > > > > > Good call from Dmitri asking you to bring the discussion on the > > list > > > > :) > > > > > > > > > > > > > > I'm +1 on the change. Returning a 500 for invalid client input > > is a > > > > bug > > > > > > > imho, not part of the API contract, and no client should rely > on > > > > getting > > > > > > a > > > > > > > 500 for a malformed request. I don't see any compatibility > > concern > > > > > > > switching to 400. Furthermore, ConnectionConfigInfo is still > > > > > > experimental, > > > > > > > so no problem to change it ;) > > > > > > > > > > > > > > I also agree with documenting authenticationParameters in the > > > > description > > > > > > > rather than adding it to required. That way the models don't > > change > > > > for > > > > > > > existing clients. > > > > > > > > > > > > > > I have just one small thing (not blocker): createCatalog in the > > > > > > Management > > > > > > > API doesn't declare a 400 response at all. The endpoint already > > > > returns > > > > > > 400 > > > > > > > for invalid storage config, and with your PR it will also do so > > for > > > > > > invalid > > > > > > > connection config, so it would be good to document it. > > > > > > > > > > > > > > I'm fine to merge if no objections here. > > > > > > > > > > > > > > Regards > > > > > > > JB > > > > > > > > > > > > > > On Thu, Nov 5, 2026 at 6:12 AM youngrae kim < > > [email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > Dmitri asked me to bring PR #5909 [1] to the list because it > > > > changes > > > > > > the > > > > > > > > HTTP status code of two > > > > > > > > Management API error responses. > > > > > > > > > > > > > > > > Today, creating an EXTERNAL catalog with an invalid > > > > > > connectionConfigInfo > > > > > > > > fails with > > > > > > > > 500 Internal Server Error in two cases, although both are > > client > > > > input > > > > > > > > errors: > > > > > > > > > > > > > > > > 1. a connectionType or authenticationType that is not in > > > > > > > > SUPPORTED_CATALOG_CONNECTION_TYPES / > > > > > > > > SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES (an > > > > > > > > IllegalStateException escapes), and > > > > > > > > 2. a request without authenticationParameters (a > > > > NullPointerException > > > > > > > > escapes; the spec only > > > > > > > > marks connectionType as required). > > > > > > > > > > > > > > > > The PR makes both return 400 Bad Request. The messages for > > case 1 > > > > are > > > > > > > > unchanged; case 2 gets > > > > > > > > "authenticationParameters is required for an external > > catalog". The > > > > > > spec > > > > > > > > description of > > > > > > > > ConnectionConfigInfo now states that authenticationParameters > > is > > > > > > > required. > > > > > > > > Valid requests are > > > > > > > > unaffected, and nothing else in the error handling changes. > > Issue: > > > > > > #5908 > > > > > > > > [2]. > > > > > > > > > > > > > > > > The PR has one approval (Dmitri). If nobody objects within a > > few > > > > days I > > > > > > > > would ask for it to be > > > > > > > > merged as is; happy to adjust if there are concerns about the > > code > > > > or > > > > > > the > > > > > > > > message. > > > > > > > > > > > > > > > > [1] https://github.com/apache/polaris/pull/5909 > > > > > > > > [2] https://github.com/apache/polaris/issues/5908 > > > > > > > > > > > > > > > > Best regards, > > > > > > > > Youngrae Kim > > > > > > > > > > > > > > > > > > > > > > > > > > > >
