You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@polaris.apache.org by Dmitri Bourlatchkov <di...@apache.org> on 2025/05/26 15:38:03 UTC
[DISCUSS] ContainerSpecHelper and the upcoming NoSQL persistence (1563)
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.
Re: [DISCUSS] ContainerSpecHelper and the upcoming NoSQL persistence (1563)
Posted by Dmitri Bourlatchkov <di...@apache.org>.
On Tue, May 27, 2025 at 12:55 PM Eric Maynard <er...@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 <fl...@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.
> > > >
> > >
> >
>
Re: [DISCUSS] ContainerSpecHelper and the upcoming NoSQL persistence (1563)
Posted by Eric Maynard <er...@gmail.com>.
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 <fl...@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.
> > >
> >
>
Re: [DISCUSS] ContainerSpecHelper and the upcoming NoSQL persistence (1563)
Posted by Dmitri Bourlatchkov <di...@apache.org>.
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 <fl...@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.
> >
>
Re: [DISCUSS] ContainerSpecHelper and the upcoming NoSQL persistence (1563)
Posted by Yufei Gu <fl...@gmail.com>.
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.
>