Hi Amogh Thanks ! Imho, I would prefer to change/"fix" the TableMetadata.Builder constructor in 1.6.0. If we release like this, It will be painful to deprecate and probably a bit confusing. I think it's better to cancel RC0 and cut RC1 including visibility change on the constructor, in order to have a "clean" 1.6.0 release.
If there are no objections, I will cancel RC0 to prepare a RC1. Regards JB On Wed, Jul 17, 2024 at 10:04 PM Amogh Jahagirdar <2am...@gmail.com> wrote: > > Hey JB, > > Yes, I'd still hold on to my -1 (non-binding) vote due to the public > TableMetadata.Builder constructor which should be private. I have a PR > https://github.com/apache/iceberg/pull/10714 for addressing it (this would > need to be cherry picked as well on to the 1.6 branch). If folks are in > agreement with that, I'd recommend another candidate. If not (which I can > understand since maybe it's a bit overkill), we could just go through a > deprecation cycle. > > Thanks, > > Amogh Jahagirdar > > On Wed, Jul 17, 2024 at 1:39 PM Jean-Baptiste Onofré <j...@nanthrax.net> > wrote: >> >> 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
