Hi Amogh Are you keeping your -1 vote ? I'm a bit lost between your two messages :)
Thanks ! Regards JB On Wed, Jul 17, 2024 at 7:03 PM Amogh Jahagirdar <2am...@gmail.com> wrote: > > 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
