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. > > >
