On Tue, May 27, 2025 at 12:55 PM Eric Maynard <eric.w.mayn...@gmail.com> wrote:
> Could we start with it scoped to the NoSQL module and promote it out when > another use case for it shows up? > > I genuinely do not understand why a module under "tools/" is a concern. I am open to discussing guidelines about the location of stuff in Polaris from the perspective of reuse and backward compatibility guarantees. However, I think, it should be done holistically. Grouping stuff under "nosql" just because the first use case is in NoSQL does not sound great to me from the overall project maintenance POV. Cheers, Dmitri. On Tue, May 27, 2025 at 9:41 AM Dmitri Bourlatchkov <di...@apache.org> > wrote: > > > 1. If ContainerSpecHelper will be used by NoSql implementation, it should > > go to the future NoSQL module. We should make it private or package scope > > if possible. > > > > > > As one can see from the name and javadoc of that class, it is not > specific > > to the > > upcoming NoSQL persistence, but it is planned to be used for testing the > > new > > persistence code. > > > > As such, the class is quite reusable within Polaris. I do not see why it > > should > > be scoped to only NoSQL code. > > > > 2. The current module in the repo introduces a public API. Any public > class > > becomes part of our compatibility contract. Once it lands, removing or > > reshaping it is a breaking change. > > > > > > That default is very strong. If we want to have a compatibility contract > I > > believe > > we ought to have a separate dev ML discussion for that. > > > > That said, the class's javadoc and location makes it clear that it is a > > testing tool. > > > > I do not think Polaris has to maintain 100% backward compatibility in its > > testing > > and tool modules. > > > > Dead code isn’t free. > > > > > > Removing this class is not free either. It will cause more work in the > > proposed > > NoSQL implementation. The fact that this class is connected to the NoSQL > > Persistence proposal does not make it dead. It is part of the gradual > > process > > of contributing NoSQL persistence in smaller chunks for ease of reviews. > > > > Cheers, > > Dmitri. > > > > On Tue, May 27, 2025 at 12:27 PM Yufei Gu <flyrain...@gmail.com> wrote: > > > > > Hi Dmitri, > > > > > > Thanks for raising this here. I have to disagree. Let me clarify my > > points. > > > 1. If ContainerSpecHelper will be used by NoSql implementation, it > should > > > go to the future NoSQL module. We should make it private or package > scope > > > if possible. > > > 2. The current module in the repo introduces a public API. Any public > > class > > > becomes part of our compatibility contract. Once it lands, removing or > > > reshaping it is a breaking change. You can already see that in the > > > 0.10.0-beta release, here > > > > > > > > > https://repository.apache.org/content/repositories/orgapachepolaris-1019/org/apache/polaris/polaris-container-spec-helper/0.10.0-beta-incubating/ > > > . > > > This doesn't only confuse users but also complicates the release > process. > > > > > > To reiterate: Dead code isn’t free. > > > > > > - Security scans still review it. > > > - Binary and license audits include it. > > > - New contributors waste time tracing unused paths. We carry that > cost > > > every release until the real feature arrives, and “temporary” code > > has a > > > habit of sticking around. > > > > > > > > > I do not think maintenance arguments raised in [1563] are relevant in > > this > > > > case as the code is planned to be used. > > > > > > Can you clarify why you think the arguments are not relevant? > > > > > > Yufei > > > > > > > > > On Mon, May 26, 2025 at 8:39 AM Dmitri Bourlatchkov <di...@apache.org> > > > wrote: > > > > > > > Hi All, > > > > > > > > PR [1563] proposes to remove ContainerSpecHelper, however it has been > > > noted > > > > to be relevant to the upcoming NoSQL implementation. > > > > > > > > AFAIK, NoSQL persistence is still on the road map. So, the removal of > > > > ContainerSpecHelper will (re-)add work to the NoSQL PRs later. > > > > > > > > I do not think maintenance arguments raised in [1563] are relevant in > > > this > > > > case as the code is planned to be used. > > > > > > > > I propose to close [1563] and if there are concerns with the location > > of > > > > that code, to address them after NoSQL persistence is merged. > > > > > > > > WDYT? > > > > > > > > [1563] https://github.com/apache/polaris/pull/1563 > > > > > > > > Thanks, > > > > Dmitri. > > > > > > > > > >
