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 > > > > > > > > > > > > > > > > >
