Could we start with it scoped to the NoSQL module and promote it out when another use case for it shows up?
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. > > > > > >
