Following up, I think I confused myself on the original issue https://github.com/apache/iceberg/issues/8756 when testing. That issue was specific to REST implementations which use `CatalogHandlers` like `RESTCatalogAdapter` used in our unit tests. The fix in #10369 does address that case for creation. When testing I was creating a v2 table and attempting to replace it with a v1 table which I think makes sense to fail because the downgrade would possibly be lossy, and then rolling back would not be safe. My original statement that "I think clients should not fail to build the change set with the format version change." is probably not correct for the downgrade case; it sounds best to fail on the client side since it's known to be unsafe.
So from a fix/issue perspective, I think we're covered. However, in terms of APIs there's still the case of the public constructor that I added in #10369. That should not be public. Thanks and sorry for the confusion there, Amogh Jahagirdar On Wed, Jul 17, 2024 at 9:48 AM Amogh Jahagirdar <2am...@gmail.com> wrote: > I'm -1 (non-binding). > > Aside from running through the standard checks, I was testing > https://github.com/apache/iceberg/pull/10369/files via Spark against a > REST catalog (a non-testing REST catalog) and the issue still exists > although the stack trace just looks a bit different now. The fix currently > handles it on the catalog handler's side which really masks the real issue > of failing to build the changes for the replace on the client side (so imo > it's not really a fix looking back on it). I'm still thinking through what > a robust solution is; in the end for REST, the service needs to be able to > handle it, but I think clients should not fail to build the change set with > the format version change. > > To be clear, I don't think I'd block on a fix for this since I'm not sure > how common of a case it is for downgrade of version for a replace is and if > there's interest in a 1.6.1, we can aim for a more thought through solution > for that release. > > However the main concern I have is when I was going through the fix, the > table metadata builder constructor I added as part of this > https://github.com/apache/iceberg/pull/10369/files#diff-c540a31e66b157a8f080433c82a29a070096d0e08c6578a0099153f1229bdb7aR913 > is marked public, which I think I'd prefer to change to private upfront > rather than have to go through a deprecation cycle/revAPI changes. > > Thanks, > > Amogh Jahagirdar > > > On Wed, Jul 17, 2024 at 2:29 AM Honah J. <hon...@apache.org> wrote: > >> +1 (non-binding) >> >> - verified signature and checksum >> - verified license doc >> - verified build and tests with JDK 17 >> >> Best regards, >> Honah >> >> On Tue, Jul 16, 2024 at 10:40 PM Ajantha Bhat <ajanthab...@gmail.com> >> wrote: >> >>> Gentle reminder for the PMC members, we need at least two additional >>>> binding votes. >>> >>> >>> One additional vote. We have binding votes from Russell and Fokko >>> already. >>> >>> On Wed, Jul 17, 2024 at 10:54 AM Jean-Baptiste Onofré <j...@nanthrax.net> >>> wrote: >>> >>>> Gentle reminder for the PMC members, we need at least two additional >>>> binding votes. >>>> >>>> Thanks ! >>>> Regards >>>> JB >>>> >>>> On Fri, Jul 12, 2024 at 4:48 PM Jean-Baptiste Onofré <j...@nanthrax.net> >>>> wrote: >>>> > >>>> > Hi everyone, >>>> > >>>> > I propose that we release the following RC as the official Apache >>>> > Iceberg 1.6.0 release. >>>> > >>>> > The commit ID is ed228f79cd3e569e04af8a8ab411811803bf3a29 >>>> > * This corresponds to the tag: apache-iceberg-1.6.0-rc0 >>>> > * https://github.com/apache/iceberg/commits/apache-iceberg-1.6.0-rc0 >>>> > * >>>> https://github.com/apache/iceberg/tree/ed228f79cd3e569e04af8a8ab411811803bf3a29 >>>> > >>>> > The release tarball, signature, and checksums are here: >>>> > * >>>> https://dist.apache.org/repos/dist/dev/iceberg/apache-iceberg-1.6.0-rc0 >>>> > >>>> > You can find the KEYS file here: >>>> > * https://dist.apache.org/repos/dist/dev/iceberg/KEYS >>>> > >>>> > Convenience binary artifacts are staged on Nexus. The Maven >>>> repository URL is: >>>> > * >>>> https://repository.apache.org/content/repositories/orgapacheiceberg-1164/ >>>> > >>>> > Please download, verify, and test. >>>> > >>>> > Please vote in the next 72 hours. >>>> > >>>> > [ ] +1 Release this as Apache Iceberg 1.6.0 >>>> > [ ] +0 >>>> > [ ] -1 Do not release this because... >>>> > >>>> > Only PMC members have binding votes, but other community members are >>>> > encouraged to cast non-binding votes. This vote will pass if there are >>>> > 3 binding +1 votes and more binding +1 votes than -1 votes. >>>> > >>>> > Thanks, >>>> > Regards >>>> > JB >>>> >>>
