+1 (binding)
On Wed, Jul 17, 2024 at 10:05 AM 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 >>>>> >>>>
