You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Yunze Xu <xy...@apache.org> on 2025/02/26 04:42:27 UTC

[DISCUSS] Do we need to update the documents for PIP and LTS releases?

Hi all,

I noticed two PRs were merged recently and cherry-picked into
branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.

https://github.com/apache/pulsar/pull/24012 fixes a bug for the
implementation of PIP-322. However, it adds a new configuration
without a new PIP or change on the existing PIP. It definitely
violates "When is a PIP required" [1]. (Any change to the
configuration)

https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
caused by geo-replication by introducing a new feature flag
`supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
feature for Pulsar brokers or client)

I also noticed https://github.com/apache/pulsar/pull/24025, which adds
a new config `managedLedgerOffloadReadThreads` as well. Though it's
only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
4.0)

I found the definition of "bug-fixes" ambiguous. It seems like a
silver bullet that if we want a PR to be included in the LTS release,
we only need to say, "it's a bug fix", no matter if it changes
anything that requires a PIP.

Based on the fact that the PIP rule is not widely followed, should we
update and loose the requirement?

[1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required

Thanks,
Yunze

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Dave Fisher <wa...@apache.org>.
From the peanut gallery.

> On Feb 26, 2025, at 12:56 AM, Lari Hotari <lh...@apache.org> wrote:
> 
> On Wed, 26 Feb 2025 at 09:47, Yunze Xu <xyz@apache.org <ma...@apache.org>> wrote:
>> 
>> Creating a PIP could emphasize that new configs are added and explain
>> why these configs are introduced. While with a regular PR, especially
>> for many recent PRs that have thousands lines of changes, the
>> configuration changes are easy to be ignored. Your question can be
>> applied for all changes required by a PIP: what's the problem for
>> changes to metrics, wire protocol APIs to users?
> 
> I agree that's a fair point. PIPs might not be the best way to capture
> the purpose of each configuration setting and when they are added. In
> many bug fix cases, a PIP was added long ago, but configurables are
> added later when it's noticed that fixing a bug requires a different
> solution which introduces a new configurable setting.

If a PIP introduces a bug that then later requires a new configurable setting to fix
that looks like the PIP is not well thought out. One hopes that the new configurables do not require setting to keep the old, expected behavior.

IMO PIPs should be tested for performance and never regress from the standard configuration.

BTW - Huntr is now a podling in the Incubator as Apache Otava, perhaps this can be incorporated into Pulsar CI?

Best,
Dave

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Lari Hotari <lh...@apache.org>.
On Wed, 26 Feb 2025 at 09:47, Yunze Xu <xy...@apache.org> wrote:
>
> Creating a PIP could emphasize that new configs are added and explain
> why these configs are introduced. While with a regular PR, especially
> for many recent PRs that have thousands lines of changes, the
> configuration changes are easy to be ignored. Your question can be
> applied for all changes required by a PIP: what's the problem for
> changes to metrics, wire protocol APIs to users?

I agree that's a fair point. PIPs might not be the best way to capture
the purpose of each configuration setting and when they are added. In
many bug fix cases, a PIP was added long ago, but configurables are
added later when it's noticed that fixing a bug requires a different
solution which introduces a new configurable setting.

It would be better to maintain configuration setting documentation in
the codebase through annotations for the configuration values, as we
already do.
The current doc annotation is used for generating documentation
visible at https://pulsar.apache.org/reference/#/4.0.x/config/reference-configuration-broker.
The problem is that all config is in one basket, and the configuration
"audience" categorization I mentioned in my previous email is missing.
The annotation could contain a longer field for detailed reasoning or
references. Perhaps that could be in the javadoc. How many of us
really look at the PIPs each time we're working on the codebase?
The source of truth for configuration should be in the codebase. The
website could be improved to display this information in different
ways. The existing
https://pulsar.apache.org/reference/#/4.0.x/config/reference-configuration-broker
is a good starting point. How many people are even aware that this
exists?

> If the rule is too strict, let's change the rule.
> If the rule is ambiguous, let's make it clear or leave an additional
> note for special cases.

One possible way to clarify things is differentiating the level of a
rule with SHOULD and MUST, as done in the ASF release policy,
https://www.apache.org/legal/release-policy.html#release-approval.
For example, "For a release vote to pass, a minimum of three positive
binding votes and more positive binding votes than negative binding
votes MUST be cast," but there's "Release votes SHOULD remain open for
at least 72 hours." In the Pulsar PIP process, I think everything is a
SHOULD.

Assuming good intentions and trusting each other will lead to good
results. Some flexibility in rule-following is necessary to deliver
value to our users. With MUST and SHOULD classifications, we can
clearly indicate which rules allow for exceptions when needed. While
ASF is often criticized for rigidity, it actually imposes relatively
few hard requirements on projects. I believe the real challenges lie
in the release policy and lack of tooling for automated releases. Once
those are addressed, most issues resolve themselves.

Adding unnecessary rigidity to Apache Pulsar would hinder the
project's ability to evolve and remain valuable to our users. Rules
should be designed with some flexibility built in. This approach may
seem counterintuitive at first, but it's a proven way to empower
contributors to achieve great results without the paralysis that comes
from fear of breaking rules. Creating psychological safety is
essential for team success. Research from Google [1, 2] shows that
teams thrive when members feel safe to take risks, speak their minds,
and be vulnerable without fear of punishment. By making it acceptable
to occasionally deviate from rules with good reason, we remove the
barriers that hold us back from innovation and enjoyment in our work.
My hope is that Apache Pulsar can be an environment where innovation
flourishes and contributors find satisfaction in their work. The
natural outcome will be delivering exceptional value to our users.

-Lari

1 - https://rework.withgoogle.com/en/guides/understanding-team-effectiveness
2 - https://www.nytimes.com/2016/02/28/magazine/what-google-learned-from-its-quest-to-build-the-perfect-team.html?smid=pl-share


>
> Another important point is that we made a rule but didn't follow the
> rule. It harms the community especially for new contributors. For
> example, IIRC, there were debates for some metrics related PRs
> cherry-picked into the LTS branch many times.
>
> If the rule is too strict, let's change the rule.
> If the rule is ambiguous, let's make it clear or leave an additional
> note for special cases.
>
> Thanks,
> Yunze
>
> On Wed, Feb 26, 2025 at 3:24 PM Lari Hotari <lh...@apache.org> wrote:
> >
> > For some of the added configurations, you are right that PIPs are needed.
> >
> > Most configurables are settings for internal implementation details
> > that don't need to be changed. The reason why configurable options are
> > added is that it's useful if the value can be modified when a user
> > faces an issue that could be solved by tuning the limit. That's
> > something that could be hard to anticipate when a change (either a bug
> > fix or new feature) is made.
> >
> > For example, PR #23901 introduced
> > managedLedgerMaxReadsInFlightPermitsAcquireTimeoutMillis and
> > managedLedgerMaxReadsInFlightPermitsAcquireQueueSize. The original
> > managedLedgerMaxReadsInFlightSizeInMB feature contained a gap where
> > the solution would never work properly when the system was under heavy
> > load and hit the limit. That's the whole point of the
> > managedLedgerMaxReadsInFlightSizeInMB solution—it adds a limit. If the
> > limit doesn't work, that's a bug. It's not a new feature when that
> > problem is fixed.
> >
> > The configs will get documented in Pulsar documentation since our
> > website will generate documentation based on the annotations in the
> > ServiceConfiguration file.
> >
> > What is the problem that these new configs have caused for our users?
> > How would creating a PIP improve the situation for our users?
> >
> > -Lari
> >
> > On Wed, 26 Feb 2025 at 06:59, Yunze Xu <xy...@apache.org> wrote:
> > >
> > > The changes to broker configs can be shown by the following command:
> > >
> > > ```
> > > git log --pretty=oneline
> > > pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
> > > ```
> > >
> > > Then I found some other PRs.
> > >
> > > https://github.com/apache/pulsar/pull/23709 adds an
> > > `allowAclChangesOnNonExistentTopics` config. However, from the comment
> > > https://github.com/apache/pulsar/pull/23709#issuecomment-2533780608,
> > > it's addressing a breaking change in
> > > https://github.com/apache/pulsar/pull/22547. Yeah, it happens again
> > > that 22547 is treated as a "bug fix" so the breaking change is
> > > acceptable for LTS releases.
> > >
> > > https://github.com/apache/pulsar/pull/22792 adds a new config
> > > `managedLedgerCursorResetLedgerCloseTimestampMaxClockSkewMillis`. It's
> > > cherry-picked to branch-4.0
> > >
> > > https://github.com/apache/pulsar/pull/20147 adds a new config. It's
> > > not cherry-picked to branch-3.0 but only as a part of the new 4.0 LTS
> > > release.
> > >
> > > ----
> > >
> > > A solution I can think of is to have an "unstable" config in addition
> > > to the existing config. The standard for which configuration change
> > > requires a PIP is really ambiguous. Pulsar's configuration now becomes
> > > really big and complicated, with many `xxxNumThreads`, `xxxTimeoutMs`
> > > configs that I cannot think of a reason in which case they should be
> > > configured.
> > >
> > > Thanks,
> > > Yunze
> > >
> > >
> > > On Wed, Feb 26, 2025 at 12:42 PM Yunze Xu <xy...@apache.org> wrote:
> > > >
> > > > Hi all,
> > > >
> > > > I noticed two PRs were merged recently and cherry-picked into
> > > > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> > > >
> > > > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > > > implementation of PIP-322. However, it adds a new configuration
> > > > without a new PIP or change on the existing PIP. It definitely
> > > > violates "When is a PIP required" [1]. (Any change to the
> > > > configuration)
> > > >
> > > > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > > > caused by geo-replication by introducing a new feature flag
> > > > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > > > feature for Pulsar brokers or client)
> > > >
> > > > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > > > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > > > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > > > 4.0)
> > > >
> > > > I found the definition of "bug-fixes" ambiguous. It seems like a
> > > > silver bullet that if we want a PR to be included in the LTS release,
> > > > we only need to say, "it's a bug fix", no matter if it changes
> > > > anything that requires a PIP.
> > > >
> > > > Based on the fact that the PIP rule is not widely followed, should we
> > > > update and loose the requirement?
> > > >
> > > > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> > > >
> > > > Thanks,
> > > > Yunze

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Yunze Xu <xy...@apache.org>.
Creating a PIP could emphasize that new configs are added and explain
why these configs are introduced. While with a regular PR, especially
for many recent PRs that have thousands lines of changes, the
configuration changes are easy to be ignored. Your question can be
applied for all changes required by a PIP: what's the problem for
changes to metrics, wire protocol APIs to users?

Another important point is that we made a rule but didn't follow the
rule. It harms the community especially for new contributors. For
example, IIRC, there were debates for some metrics related PRs
cherry-picked into the LTS branch many times.

If the rule is too strict, let's change the rule.
If the rule is ambiguous, let's make it clear or leave an additional
note for special cases.

Thanks,
Yunze

On Wed, Feb 26, 2025 at 3:24 PM Lari Hotari <lh...@apache.org> wrote:
>
> For some of the added configurations, you are right that PIPs are needed.
>
> Most configurables are settings for internal implementation details
> that don't need to be changed. The reason why configurable options are
> added is that it's useful if the value can be modified when a user
> faces an issue that could be solved by tuning the limit. That's
> something that could be hard to anticipate when a change (either a bug
> fix or new feature) is made.
>
> For example, PR #23901 introduced
> managedLedgerMaxReadsInFlightPermitsAcquireTimeoutMillis and
> managedLedgerMaxReadsInFlightPermitsAcquireQueueSize. The original
> managedLedgerMaxReadsInFlightSizeInMB feature contained a gap where
> the solution would never work properly when the system was under heavy
> load and hit the limit. That's the whole point of the
> managedLedgerMaxReadsInFlightSizeInMB solution—it adds a limit. If the
> limit doesn't work, that's a bug. It's not a new feature when that
> problem is fixed.
>
> The configs will get documented in Pulsar documentation since our
> website will generate documentation based on the annotations in the
> ServiceConfiguration file.
>
> What is the problem that these new configs have caused for our users?
> How would creating a PIP improve the situation for our users?
>
> -Lari
>
> On Wed, 26 Feb 2025 at 06:59, Yunze Xu <xy...@apache.org> wrote:
> >
> > The changes to broker configs can be shown by the following command:
> >
> > ```
> > git log --pretty=oneline
> > pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
> > ```
> >
> > Then I found some other PRs.
> >
> > https://github.com/apache/pulsar/pull/23709 adds an
> > `allowAclChangesOnNonExistentTopics` config. However, from the comment
> > https://github.com/apache/pulsar/pull/23709#issuecomment-2533780608,
> > it's addressing a breaking change in
> > https://github.com/apache/pulsar/pull/22547. Yeah, it happens again
> > that 22547 is treated as a "bug fix" so the breaking change is
> > acceptable for LTS releases.
> >
> > https://github.com/apache/pulsar/pull/22792 adds a new config
> > `managedLedgerCursorResetLedgerCloseTimestampMaxClockSkewMillis`. It's
> > cherry-picked to branch-4.0
> >
> > https://github.com/apache/pulsar/pull/20147 adds a new config. It's
> > not cherry-picked to branch-3.0 but only as a part of the new 4.0 LTS
> > release.
> >
> > ----
> >
> > A solution I can think of is to have an "unstable" config in addition
> > to the existing config. The standard for which configuration change
> > requires a PIP is really ambiguous. Pulsar's configuration now becomes
> > really big and complicated, with many `xxxNumThreads`, `xxxTimeoutMs`
> > configs that I cannot think of a reason in which case they should be
> > configured.
> >
> > Thanks,
> > Yunze
> >
> >
> > On Wed, Feb 26, 2025 at 12:42 PM Yunze Xu <xy...@apache.org> wrote:
> > >
> > > Hi all,
> > >
> > > I noticed two PRs were merged recently and cherry-picked into
> > > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> > >
> > > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > > implementation of PIP-322. However, it adds a new configuration
> > > without a new PIP or change on the existing PIP. It definitely
> > > violates "When is a PIP required" [1]. (Any change to the
> > > configuration)
> > >
> > > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > > caused by geo-replication by introducing a new feature flag
> > > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > > feature for Pulsar brokers or client)
> > >
> > > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > > 4.0)
> > >
> > > I found the definition of "bug-fixes" ambiguous. It seems like a
> > > silver bullet that if we want a PR to be included in the LTS release,
> > > we only need to say, "it's a bug fix", no matter if it changes
> > > anything that requires a PIP.
> > >
> > > Based on the fact that the PIP rule is not widely followed, should we
> > > update and loose the requirement?
> > >
> > > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> > >
> > > Thanks,
> > > Yunze

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Lari Hotari <lh...@apache.org>.
For some of the added configurations, you are right that PIPs are needed.

Most configurables are settings for internal implementation details
that don't need to be changed. The reason why configurable options are
added is that it's useful if the value can be modified when a user
faces an issue that could be solved by tuning the limit. That's
something that could be hard to anticipate when a change (either a bug
fix or new feature) is made.

For example, PR #23901 introduced
managedLedgerMaxReadsInFlightPermitsAcquireTimeoutMillis and
managedLedgerMaxReadsInFlightPermitsAcquireQueueSize. The original
managedLedgerMaxReadsInFlightSizeInMB feature contained a gap where
the solution would never work properly when the system was under heavy
load and hit the limit. That's the whole point of the
managedLedgerMaxReadsInFlightSizeInMB solution—it adds a limit. If the
limit doesn't work, that's a bug. It's not a new feature when that
problem is fixed.

The configs will get documented in Pulsar documentation since our
website will generate documentation based on the annotations in the
ServiceConfiguration file.

What is the problem that these new configs have caused for our users?
How would creating a PIP improve the situation for our users?

-Lari

On Wed, 26 Feb 2025 at 06:59, Yunze Xu <xy...@apache.org> wrote:
>
> The changes to broker configs can be shown by the following command:
>
> ```
> git log --pretty=oneline
> pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
> ```
>
> Then I found some other PRs.
>
> https://github.com/apache/pulsar/pull/23709 adds an
> `allowAclChangesOnNonExistentTopics` config. However, from the comment
> https://github.com/apache/pulsar/pull/23709#issuecomment-2533780608,
> it's addressing a breaking change in
> https://github.com/apache/pulsar/pull/22547. Yeah, it happens again
> that 22547 is treated as a "bug fix" so the breaking change is
> acceptable for LTS releases.
>
> https://github.com/apache/pulsar/pull/22792 adds a new config
> `managedLedgerCursorResetLedgerCloseTimestampMaxClockSkewMillis`. It's
> cherry-picked to branch-4.0
>
> https://github.com/apache/pulsar/pull/20147 adds a new config. It's
> not cherry-picked to branch-3.0 but only as a part of the new 4.0 LTS
> release.
>
> ----
>
> A solution I can think of is to have an "unstable" config in addition
> to the existing config. The standard for which configuration change
> requires a PIP is really ambiguous. Pulsar's configuration now becomes
> really big and complicated, with many `xxxNumThreads`, `xxxTimeoutMs`
> configs that I cannot think of a reason in which case they should be
> configured.
>
> Thanks,
> Yunze
>
>
> On Wed, Feb 26, 2025 at 12:42 PM Yunze Xu <xy...@apache.org> wrote:
> >
> > Hi all,
> >
> > I noticed two PRs were merged recently and cherry-picked into
> > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> >
> > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > implementation of PIP-322. However, it adds a new configuration
> > without a new PIP or change on the existing PIP. It definitely
> > violates "When is a PIP required" [1]. (Any change to the
> > configuration)
> >
> > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > caused by geo-replication by introducing a new feature flag
> > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > feature for Pulsar brokers or client)
> >
> > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > 4.0)
> >
> > I found the definition of "bug-fixes" ambiguous. It seems like a
> > silver bullet that if we want a PR to be included in the LTS release,
> > we only need to say, "it's a bug fix", no matter if it changes
> > anything that requires a PIP.
> >
> > Based on the fact that the PIP rule is not widely followed, should we
> > update and loose the requirement?
> >
> > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> >
> > Thanks,
> > Yunze

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Yunze Xu <xy...@apache.org>.
The changes to broker configs can be shown by the following command:

```
git log --pretty=oneline
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
```

Then I found some other PRs.

https://github.com/apache/pulsar/pull/23709 adds an
`allowAclChangesOnNonExistentTopics` config. However, from the comment
https://github.com/apache/pulsar/pull/23709#issuecomment-2533780608,
it's addressing a breaking change in
https://github.com/apache/pulsar/pull/22547. Yeah, it happens again
that 22547 is treated as a "bug fix" so the breaking change is
acceptable for LTS releases.

https://github.com/apache/pulsar/pull/22792 adds a new config
`managedLedgerCursorResetLedgerCloseTimestampMaxClockSkewMillis`. It's
cherry-picked to branch-4.0

https://github.com/apache/pulsar/pull/20147 adds a new config. It's
not cherry-picked to branch-3.0 but only as a part of the new 4.0 LTS
release.

----

A solution I can think of is to have an "unstable" config in addition
to the existing config. The standard for which configuration change
requires a PIP is really ambiguous. Pulsar's configuration now becomes
really big and complicated, with many `xxxNumThreads`, `xxxTimeoutMs`
configs that I cannot think of a reason in which case they should be
configured.

Thanks,
Yunze


On Wed, Feb 26, 2025 at 12:42 PM Yunze Xu <xy...@apache.org> wrote:
>
> Hi all,
>
> I noticed two PRs were merged recently and cherry-picked into
> branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
>
> https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> implementation of PIP-322. However, it adds a new configuration
> without a new PIP or change on the existing PIP. It definitely
> violates "When is a PIP required" [1]. (Any change to the
> configuration)
>
> https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> caused by geo-replication by introducing a new feature flag
> `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> feature for Pulsar brokers or client)
>
> I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> a new config `managedLedgerOffloadReadThreads` as well. Though it's
> only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> 4.0)
>
> I found the definition of "bug-fixes" ambiguous. It seems like a
> silver bullet that if we want a PR to be included in the LTS release,
> we only need to say, "it's a bug fix", no matter if it changes
> anything that requires a PIP.
>
> Based on the fact that the PIP rule is not widely followed, should we
> update and loose the requirement?
>
> [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
>
> Thanks,
> Yunze

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Yunze Xu <xy...@apache.org>.
IMO, https://pulsar.apache.org/reference/#/4.0.x/config/reference-configuration-broker
is not much more readable than the broker.conf, which is a part of the
release. From my experience, users tend to take a look at the
configuration files in the release first.

Based on your points, the committers should be responsible to verify
if a new configuration or a new public API needs a PIP now. Okay, then
committers would have the power to merge any PR quickly and pick it to
the LTS release. We'll see endless new configurations, even the API
changes to the LTS release without any discussion on the mail list.

I'd like to provide some typical exceptional cases in the documents.
For example, your comment here [1] is reasonable for adding a new
config in this PR. IMO, at least, even without a PIP, any
configuration change MUST have a public discussion on the mail list.

> Rules should be designed with some flexibility built in.

I only hope "flexibility" does not mean "double standards". After
checking it again, I found https://github.com/apache/pulsar/pull/20147
has an associated PIP-271. But this change is just a very simple
configuration to customize a timeout of a delayed task. While your
huge PR [2] adds two configs without any PIP and it's merged quickly
and cherry-picked into the LTS release.

@rdhabalia expressed it directly and frankly under that PR [3]: "You
block PRs from others without any reason, and here I tried to avoid
that destructive practice and didn't block it but you just merged it."

> However, all of them shouldn't be made "hard coded" constants in code.

Well, I searched the usages of `ScheduledExecutorService#schedule`
only in the pulsar-broker module and found 30+ places (tests are
excluded) it's called. Most of them are hard coded. Are you sure that
all these delayed time arguments should be configurable? If so, how
complicated could it be to tune the configs? Any committer can push a
new PR to add all configurations for these arguments. What's worse, it
could be cherry-picked into branch-3.0 if he/she wants with a simple
explanation like "it makes LTS users enjoy tuning Pulsar with a better
control".

> Adding more processes and rules or removing configs won't clean up the mess.

It's not a fix for existing issues. It prevents things from being worse.

[1] https://github.com/apache/pulsar/pull/23709#issuecomment-2533780608
[2] https://github.com/apache/pulsar/pull/24012
[3] https://github.com/apache/pulsar/pull/24012#issuecomment-2682552507

Thanks,
Yunze


On Wed, Feb 26, 2025 at 5:15 PM Lari Hotari <lh...@apache.org> wrote:
>
> On Wed, 26 Feb 2025 at 10:55, Yunze Xu <xy...@apache.org> wrote:
> >
> > Removing these configurations is helpful indeed. Once it's included in
> > a release, removing them could be much harder. This PR also reminds
> > the authors: are these configurations really necessary? If so, just
> > use a separate PR to explain your reason and add these configs back
> > again. PIP is suitable for that. If the process of PIP is a formalism
> > that takes much time, I agree with just using a separate simple PR for
> > the purpose, as well as updating the PIP process.
>
> I have replied in a separate email about the process and rigidity aspects.
>
> > Providing some configs for users to tune is unrealistic. It's exactly
> > equivalent to provide an unstable feature and tell users it could be
> > tuned to a good situation, with no actual testing. For example, a
> > delayed task has a configurable timeout (assuming it's 60 seconds).
> > From the perspective of a Pulsar vendor, when the customer reported an
> > issue, there is no chance for the vendor to tune configs, which means
> > the whole cluster needs to be restarted again and again until a good
> > situation is reached.
>
> I don't disagree. I believe that the configuration tunable
> categorization (audience, stability) could address most of this from
> the users perspective so that internal configs could be hidden from
> top level documentation. In many cases "hard coding" a configuration
> value is an absolute bad practice. There just needs to be a way to
> configure implementation level details which aren't intended for our
> users.
> Changing a value of certain settings is a last resort. However, all of
> them shouldn't be made "hard coded" constants in code.
>
> > The original motivation of this mail list is not related to how to
> > improve the config. It's more like a discussion if we insist on adding
> > configurations endlessly without a formal process. We prevented
> > changes from many contributors and told them we don't want to make
> > configurations more complicated.
>
> I believe that improving the configuration is needed. Adding more
> processes and rules or removing configs won't clean up the mess.
> I have replied about the process in a previous email.
>
> > If it's not the truth, don't pretend to declare something like LTS.
> > Just document it clearly: welcome to contribute configurations for
> > users to tune and we'll resolve the issue in (an uncertain) future.
>
> Improving the LTS process is another important matter. Regarding
> "rules", I covered that in my previous emails.
>
> -Lari
>
> >
> > [1] https://github.com/apache/pulsar/pull/24027#pullrequestreview-2643354724
> >
> > Thanks,
> > Yunze
> >
> > On Wed, Feb 26, 2025 at 4:08 PM Lari Hotari <lh...@apache.org> wrote:
> > >
> > > On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
> > > >
> > > > I opened a PR to clean up some recent config changes:
> > > > https://github.com/apache/pulsar/pull/24027
> > >
> > > I don't think that removing the configuration values would be helpful,
> > > though I appreciate that your PR was an effective way to create
> > > urgency around solving this problem. I agree with you that the current
> > > situation is a mess. I value your efforts to drive improvements in
> > > this area, so please don't interpret my comments as resistance to
> > > change. The problem extends beyond what we've discussed in this thread
> > > so far.
> > >
> > > In an earlier email to the list [1], I brought up some other issues
> > > which are related to how configuration is applied in Docker
> > > containers.
> > > There's a very dirty solution to modify the config files using Python
> > > scripts in Pulsar docker containers, such as
> > > "bin/apply-config-from-env.py conf/broker.conf" and
> > > "bin/gen-yml-from-env.py conf/functions_worker.yml". The earlier email
> > > was focusing on solving that problem by selecting a new configuration
> > > library that supports multiple config sources and could directly use
> > > environment variables or other sources.
> > >
> > > Another reason why the Pulsar configuration is a mess is that it's all
> > > consolidated in a single location, ServiceConfiguration. From an
> > > object-oriented design perspective, ServiceConfiguration has become a
> > > "God Object" [2] in the codebase, which creates its own set of
> > > problems. In more modern configuration approaches, configuration for
> > > modules is kept within each module and is designed to be hierarchical
> > > and modular. For example, Spring Boot's configuration solution (when
> > > used correctly) supports this approach.
> > >
> > > In our APIs, we have a classification of audience: Public,
> > > LimitedPrivate, and Private with the
> > > org.apache.pulsar.common.classification.InterfaceAudience annotation
> > > and stability classification of Stable, Evolving, and Unstable.
> > > Perhaps configuration options could also be categorized as "Private"
> > > when they're settings that shouldn't be primarily configured by end
> > > users and are tunables for implementation level details.
> > >
> > > It seems that "PIP-346: Add a simplified configuration file for
> > > Pulsar" [3] intends to solve categorization with
> > > "broker-defaults.conf". I initially resisted that approach so that we
> > > would address the broader problem and not just add a patch to the
> > > current solution. Patching could be a way forward if we aren't able to
> > > conclude on long-term improvements and cleaning up the mess. My
> > > concern is that a point solution could just add more complexity
> > > without resolving the problems around configuration modularity and
> > > hierarchy.
> > >
> > > > Pulsar's configuration is like a trashbox, to which everyone throws
> > > > trash without any consideration. Some configs are hard to understand
> > > > without looking into the implementation details. The PIP requirement
> > > > should prevent such things but it seems that very few contributors
> > > > follow it.
> > >
> > > This is the reason why separating implementation-level detail configs
> > > ("Private" audience) and user-level configs ("Public" audience) could
> > > be helpful. I guess that a solution in the direction of PIP-346 could
> > > help with this categorization in addition to using annotations in the
> > > ServiceConfiguration file. I wonder if instead of "broker.conf" and
> > > "broker-defaults.conf", there could be two separate files:
> > > "broker.conf" for public settings and "broker-internal.conf" for the
> > > internal implementation-level detail settings. However, that could
> > > introduce new problems such as issues with the
> > > "apply-config-from-env.py" script and Pulsar docker container
> > > configuration.
> > >
> > > -Lari
> > >
> > > 1 - https://lists.apache.org/thread/8splwyrn25pt5rb6ph0yz0tfgknn3hqh
> > > 2 - https://en.wikipedia.org/wiki/God_object
> > > 3 - https://github.com/apache/pulsar/pull/22274/files
> > >
> > >
> > > On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
> > > >
> > > > I opened a PR to clean up some recent config changes:
> > > > https://github.com/apache/pulsar/pull/24027
> > > >
> > > > Pulsar's configuration is like a trashbox, to which everyone throws
> > > > trash without any consideration. Some configs are hard to understand
> > > > without looking into the implementation details. The PIP requirement
> > > > should prevent such things but it seems that very few contributors
> > > > follow it.
> > > >
> > > > I raised my concern two years ago [1] but things are getting worse.
> > > > IMHO, most configs do not have any value to exist. For these configs,
> > > > no one knows when it should be changed and if the customized value
> > > > could be better. AUthors tend to have no responsibility for the config
> > > > added by them. They just think, "you should thank me for providing a
> > > > config to control the behavior". They never stand from a perspective
> > > > of users.
> > > >
> > > > [1] https://lists.apache.org/thread/j23ny19opmp8jww57gwk7g27b5dvl0ot
> > > >
> > > > Thanks,
> > > > Yunze
> > > >
> > > > On Wed, Feb 26, 2025 at 2:33 PM WenZhi Feng <th...@apache.org> wrote:
> > > > >
> > > > > Same confusion about the criterion of PIP.
> > > > >
> > > > > Wenzhi Feng.
> > > > >
> > > > > On 2025/02/26 04:42:27 Yunze Xu wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > I noticed two PRs were merged recently and cherry-picked into
> > > > > > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> > > > > >
> > > > > > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > > > > > implementation of PIP-322. However, it adds a new configuration
> > > > > > without a new PIP or change on the existing PIP. It definitely
> > > > > > violates "When is a PIP required" [1]. (Any change to the
> > > > > > configuration)
> > > > > >
> > > > > > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > > > > > caused by geo-replication by introducing a new feature flag
> > > > > > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > > > > > feature for Pulsar brokers or client)
> > > > > >
> > > > > > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > > > > > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > > > > > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > > > > > 4.0)
> > > > > >
> > > > > > I found the definition of "bug-fixes" ambiguous. It seems like a
> > > > > > silver bullet that if we want a PR to be included in the LTS release,
> > > > > > we only need to say, "it's a bug fix", no matter if it changes
> > > > > > anything that requires a PIP.
> > > > > >
> > > > > > Based on the fact that the PIP rule is not widely followed, should we
> > > > > > update and loose the requirement?
> > > > > >
> > > > > > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> > > > > >
> > > > > > Thanks,
> > > > > > Yunze
> > > > > >

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Lari Hotari <lh...@apache.org>.
On Wed, 26 Feb 2025 at 10:55, Yunze Xu <xy...@apache.org> wrote:
>
> Removing these configurations is helpful indeed. Once it's included in
> a release, removing them could be much harder. This PR also reminds
> the authors: are these configurations really necessary? If so, just
> use a separate PR to explain your reason and add these configs back
> again. PIP is suitable for that. If the process of PIP is a formalism
> that takes much time, I agree with just using a separate simple PR for
> the purpose, as well as updating the PIP process.

I have replied in a separate email about the process and rigidity aspects.

> Providing some configs for users to tune is unrealistic. It's exactly
> equivalent to provide an unstable feature and tell users it could be
> tuned to a good situation, with no actual testing. For example, a
> delayed task has a configurable timeout (assuming it's 60 seconds).
> From the perspective of a Pulsar vendor, when the customer reported an
> issue, there is no chance for the vendor to tune configs, which means
> the whole cluster needs to be restarted again and again until a good
> situation is reached.

I don't disagree. I believe that the configuration tunable
categorization (audience, stability) could address most of this from
the users perspective so that internal configs could be hidden from
top level documentation. In many cases "hard coding" a configuration
value is an absolute bad practice. There just needs to be a way to
configure implementation level details which aren't intended for our
users.
Changing a value of certain settings is a last resort. However, all of
them shouldn't be made "hard coded" constants in code.

> The original motivation of this mail list is not related to how to
> improve the config. It's more like a discussion if we insist on adding
> configurations endlessly without a formal process. We prevented
> changes from many contributors and told them we don't want to make
> configurations more complicated.

I believe that improving the configuration is needed. Adding more
processes and rules or removing configs won't clean up the mess.
I have replied about the process in a previous email.

> If it's not the truth, don't pretend to declare something like LTS.
> Just document it clearly: welcome to contribute configurations for
> users to tune and we'll resolve the issue in (an uncertain) future.

Improving the LTS process is another important matter. Regarding
"rules", I covered that in my previous emails.

-Lari

>
> [1] https://github.com/apache/pulsar/pull/24027#pullrequestreview-2643354724
>
> Thanks,
> Yunze
>
> On Wed, Feb 26, 2025 at 4:08 PM Lari Hotari <lh...@apache.org> wrote:
> >
> > On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
> > >
> > > I opened a PR to clean up some recent config changes:
> > > https://github.com/apache/pulsar/pull/24027
> >
> > I don't think that removing the configuration values would be helpful,
> > though I appreciate that your PR was an effective way to create
> > urgency around solving this problem. I agree with you that the current
> > situation is a mess. I value your efforts to drive improvements in
> > this area, so please don't interpret my comments as resistance to
> > change. The problem extends beyond what we've discussed in this thread
> > so far.
> >
> > In an earlier email to the list [1], I brought up some other issues
> > which are related to how configuration is applied in Docker
> > containers.
> > There's a very dirty solution to modify the config files using Python
> > scripts in Pulsar docker containers, such as
> > "bin/apply-config-from-env.py conf/broker.conf" and
> > "bin/gen-yml-from-env.py conf/functions_worker.yml". The earlier email
> > was focusing on solving that problem by selecting a new configuration
> > library that supports multiple config sources and could directly use
> > environment variables or other sources.
> >
> > Another reason why the Pulsar configuration is a mess is that it's all
> > consolidated in a single location, ServiceConfiguration. From an
> > object-oriented design perspective, ServiceConfiguration has become a
> > "God Object" [2] in the codebase, which creates its own set of
> > problems. In more modern configuration approaches, configuration for
> > modules is kept within each module and is designed to be hierarchical
> > and modular. For example, Spring Boot's configuration solution (when
> > used correctly) supports this approach.
> >
> > In our APIs, we have a classification of audience: Public,
> > LimitedPrivate, and Private with the
> > org.apache.pulsar.common.classification.InterfaceAudience annotation
> > and stability classification of Stable, Evolving, and Unstable.
> > Perhaps configuration options could also be categorized as "Private"
> > when they're settings that shouldn't be primarily configured by end
> > users and are tunables for implementation level details.
> >
> > It seems that "PIP-346: Add a simplified configuration file for
> > Pulsar" [3] intends to solve categorization with
> > "broker-defaults.conf". I initially resisted that approach so that we
> > would address the broader problem and not just add a patch to the
> > current solution. Patching could be a way forward if we aren't able to
> > conclude on long-term improvements and cleaning up the mess. My
> > concern is that a point solution could just add more complexity
> > without resolving the problems around configuration modularity and
> > hierarchy.
> >
> > > Pulsar's configuration is like a trashbox, to which everyone throws
> > > trash without any consideration. Some configs are hard to understand
> > > without looking into the implementation details. The PIP requirement
> > > should prevent such things but it seems that very few contributors
> > > follow it.
> >
> > This is the reason why separating implementation-level detail configs
> > ("Private" audience) and user-level configs ("Public" audience) could
> > be helpful. I guess that a solution in the direction of PIP-346 could
> > help with this categorization in addition to using annotations in the
> > ServiceConfiguration file. I wonder if instead of "broker.conf" and
> > "broker-defaults.conf", there could be two separate files:
> > "broker.conf" for public settings and "broker-internal.conf" for the
> > internal implementation-level detail settings. However, that could
> > introduce new problems such as issues with the
> > "apply-config-from-env.py" script and Pulsar docker container
> > configuration.
> >
> > -Lari
> >
> > 1 - https://lists.apache.org/thread/8splwyrn25pt5rb6ph0yz0tfgknn3hqh
> > 2 - https://en.wikipedia.org/wiki/God_object
> > 3 - https://github.com/apache/pulsar/pull/22274/files
> >
> >
> > On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
> > >
> > > I opened a PR to clean up some recent config changes:
> > > https://github.com/apache/pulsar/pull/24027
> > >
> > > Pulsar's configuration is like a trashbox, to which everyone throws
> > > trash without any consideration. Some configs are hard to understand
> > > without looking into the implementation details. The PIP requirement
> > > should prevent such things but it seems that very few contributors
> > > follow it.
> > >
> > > I raised my concern two years ago [1] but things are getting worse.
> > > IMHO, most configs do not have any value to exist. For these configs,
> > > no one knows when it should be changed and if the customized value
> > > could be better. AUthors tend to have no responsibility for the config
> > > added by them. They just think, "you should thank me for providing a
> > > config to control the behavior". They never stand from a perspective
> > > of users.
> > >
> > > [1] https://lists.apache.org/thread/j23ny19opmp8jww57gwk7g27b5dvl0ot
> > >
> > > Thanks,
> > > Yunze
> > >
> > > On Wed, Feb 26, 2025 at 2:33 PM WenZhi Feng <th...@apache.org> wrote:
> > > >
> > > > Same confusion about the criterion of PIP.
> > > >
> > > > Wenzhi Feng.
> > > >
> > > > On 2025/02/26 04:42:27 Yunze Xu wrote:
> > > > > Hi all,
> > > > >
> > > > > I noticed two PRs were merged recently and cherry-picked into
> > > > > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> > > > >
> > > > > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > > > > implementation of PIP-322. However, it adds a new configuration
> > > > > without a new PIP or change on the existing PIP. It definitely
> > > > > violates "When is a PIP required" [1]. (Any change to the
> > > > > configuration)
> > > > >
> > > > > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > > > > caused by geo-replication by introducing a new feature flag
> > > > > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > > > > feature for Pulsar brokers or client)
> > > > >
> > > > > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > > > > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > > > > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > > > > 4.0)
> > > > >
> > > > > I found the definition of "bug-fixes" ambiguous. It seems like a
> > > > > silver bullet that if we want a PR to be included in the LTS release,
> > > > > we only need to say, "it's a bug fix", no matter if it changes
> > > > > anything that requires a PIP.
> > > > >
> > > > > Based on the fact that the PIP rule is not widely followed, should we
> > > > > update and loose the requirement?
> > > > >
> > > > > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> > > > >
> > > > > Thanks,
> > > > > Yunze
> > > > >

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Yunze Xu <xy...@apache.org>.
Removing these configurations is helpful indeed. Once it's included in
a release, removing them could be much harder. This PR also reminds
the authors: are these configurations really necessary? If so, just
use a separate PR to explain your reason and add these configs back
again. PIP is suitable for that. If the process of PIP is a formalism
that takes much time, I agree with just using a separate simple PR for
the purpose, as well as updating the PIP process.

Providing some configs for users to tune is unrealistic. It's exactly
equivalent to provide an unstable feature and tell users it could be
tuned to a good situation, with no actual testing. For example, a
delayed task has a configurable timeout (assuming it's 60 seconds).
From the perspective of a Pulsar vendor, when the customer reported an
issue, there is no chance for the vendor to tune configs, which means
the whole cluster needs to be restarted again and again until a good
situation is reached.

The original motivation of this mail list is not related to how to
improve the config. It's more like a discussion if we insist on adding
configurations endlessly without a formal process. We prevented
changes from many contributors and told them we don't want to make
configurations more complicated.

If it's not the truth, don't pretend to declare something like LTS.
Just document it clearly: welcome to contribute configurations for
users to tune and we'll resolve the issue in (an uncertain) future.

[1] https://github.com/apache/pulsar/pull/24027#pullrequestreview-2643354724

Thanks,
Yunze

On Wed, Feb 26, 2025 at 4:08 PM Lari Hotari <lh...@apache.org> wrote:
>
> On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
> >
> > I opened a PR to clean up some recent config changes:
> > https://github.com/apache/pulsar/pull/24027
>
> I don't think that removing the configuration values would be helpful,
> though I appreciate that your PR was an effective way to create
> urgency around solving this problem. I agree with you that the current
> situation is a mess. I value your efforts to drive improvements in
> this area, so please don't interpret my comments as resistance to
> change. The problem extends beyond what we've discussed in this thread
> so far.
>
> In an earlier email to the list [1], I brought up some other issues
> which are related to how configuration is applied in Docker
> containers.
> There's a very dirty solution to modify the config files using Python
> scripts in Pulsar docker containers, such as
> "bin/apply-config-from-env.py conf/broker.conf" and
> "bin/gen-yml-from-env.py conf/functions_worker.yml". The earlier email
> was focusing on solving that problem by selecting a new configuration
> library that supports multiple config sources and could directly use
> environment variables or other sources.
>
> Another reason why the Pulsar configuration is a mess is that it's all
> consolidated in a single location, ServiceConfiguration. From an
> object-oriented design perspective, ServiceConfiguration has become a
> "God Object" [2] in the codebase, which creates its own set of
> problems. In more modern configuration approaches, configuration for
> modules is kept within each module and is designed to be hierarchical
> and modular. For example, Spring Boot's configuration solution (when
> used correctly) supports this approach.
>
> In our APIs, we have a classification of audience: Public,
> LimitedPrivate, and Private with the
> org.apache.pulsar.common.classification.InterfaceAudience annotation
> and stability classification of Stable, Evolving, and Unstable.
> Perhaps configuration options could also be categorized as "Private"
> when they're settings that shouldn't be primarily configured by end
> users and are tunables for implementation level details.
>
> It seems that "PIP-346: Add a simplified configuration file for
> Pulsar" [3] intends to solve categorization with
> "broker-defaults.conf". I initially resisted that approach so that we
> would address the broader problem and not just add a patch to the
> current solution. Patching could be a way forward if we aren't able to
> conclude on long-term improvements and cleaning up the mess. My
> concern is that a point solution could just add more complexity
> without resolving the problems around configuration modularity and
> hierarchy.
>
> > Pulsar's configuration is like a trashbox, to which everyone throws
> > trash without any consideration. Some configs are hard to understand
> > without looking into the implementation details. The PIP requirement
> > should prevent such things but it seems that very few contributors
> > follow it.
>
> This is the reason why separating implementation-level detail configs
> ("Private" audience) and user-level configs ("Public" audience) could
> be helpful. I guess that a solution in the direction of PIP-346 could
> help with this categorization in addition to using annotations in the
> ServiceConfiguration file. I wonder if instead of "broker.conf" and
> "broker-defaults.conf", there could be two separate files:
> "broker.conf" for public settings and "broker-internal.conf" for the
> internal implementation-level detail settings. However, that could
> introduce new problems such as issues with the
> "apply-config-from-env.py" script and Pulsar docker container
> configuration.
>
> -Lari
>
> 1 - https://lists.apache.org/thread/8splwyrn25pt5rb6ph0yz0tfgknn3hqh
> 2 - https://en.wikipedia.org/wiki/God_object
> 3 - https://github.com/apache/pulsar/pull/22274/files
>
>
> On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
> >
> > I opened a PR to clean up some recent config changes:
> > https://github.com/apache/pulsar/pull/24027
> >
> > Pulsar's configuration is like a trashbox, to which everyone throws
> > trash without any consideration. Some configs are hard to understand
> > without looking into the implementation details. The PIP requirement
> > should prevent such things but it seems that very few contributors
> > follow it.
> >
> > I raised my concern two years ago [1] but things are getting worse.
> > IMHO, most configs do not have any value to exist. For these configs,
> > no one knows when it should be changed and if the customized value
> > could be better. AUthors tend to have no responsibility for the config
> > added by them. They just think, "you should thank me for providing a
> > config to control the behavior". They never stand from a perspective
> > of users.
> >
> > [1] https://lists.apache.org/thread/j23ny19opmp8jww57gwk7g27b5dvl0ot
> >
> > Thanks,
> > Yunze
> >
> > On Wed, Feb 26, 2025 at 2:33 PM WenZhi Feng <th...@apache.org> wrote:
> > >
> > > Same confusion about the criterion of PIP.
> > >
> > > Wenzhi Feng.
> > >
> > > On 2025/02/26 04:42:27 Yunze Xu wrote:
> > > > Hi all,
> > > >
> > > > I noticed two PRs were merged recently and cherry-picked into
> > > > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> > > >
> > > > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > > > implementation of PIP-322. However, it adds a new configuration
> > > > without a new PIP or change on the existing PIP. It definitely
> > > > violates "When is a PIP required" [1]. (Any change to the
> > > > configuration)
> > > >
> > > > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > > > caused by geo-replication by introducing a new feature flag
> > > > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > > > feature for Pulsar brokers or client)
> > > >
> > > > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > > > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > > > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > > > 4.0)
> > > >
> > > > I found the definition of "bug-fixes" ambiguous. It seems like a
> > > > silver bullet that if we want a PR to be included in the LTS release,
> > > > we only need to say, "it's a bug fix", no matter if it changes
> > > > anything that requires a PIP.
> > > >
> > > > Based on the fact that the PIP rule is not widely followed, should we
> > > > update and loose the requirement?
> > > >
> > > > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> > > >
> > > > Thanks,
> > > > Yunze
> > > >

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Lari Hotari <lh...@apache.org>.
On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
>
> I opened a PR to clean up some recent config changes:
> https://github.com/apache/pulsar/pull/24027

I don't think that removing the configuration values would be helpful,
though I appreciate that your PR was an effective way to create
urgency around solving this problem. I agree with you that the current
situation is a mess. I value your efforts to drive improvements in
this area, so please don't interpret my comments as resistance to
change. The problem extends beyond what we've discussed in this thread
so far.

In an earlier email to the list [1], I brought up some other issues
which are related to how configuration is applied in Docker
containers.
There's a very dirty solution to modify the config files using Python
scripts in Pulsar docker containers, such as
"bin/apply-config-from-env.py conf/broker.conf" and
"bin/gen-yml-from-env.py conf/functions_worker.yml". The earlier email
was focusing on solving that problem by selecting a new configuration
library that supports multiple config sources and could directly use
environment variables or other sources.

Another reason why the Pulsar configuration is a mess is that it's all
consolidated in a single location, ServiceConfiguration. From an
object-oriented design perspective, ServiceConfiguration has become a
"God Object" [2] in the codebase, which creates its own set of
problems. In more modern configuration approaches, configuration for
modules is kept within each module and is designed to be hierarchical
and modular. For example, Spring Boot's configuration solution (when
used correctly) supports this approach.

In our APIs, we have a classification of audience: Public,
LimitedPrivate, and Private with the
org.apache.pulsar.common.classification.InterfaceAudience annotation
and stability classification of Stable, Evolving, and Unstable.
Perhaps configuration options could also be categorized as "Private"
when they're settings that shouldn't be primarily configured by end
users and are tunables for implementation level details.

It seems that "PIP-346: Add a simplified configuration file for
Pulsar" [3] intends to solve categorization with
"broker-defaults.conf". I initially resisted that approach so that we
would address the broader problem and not just add a patch to the
current solution. Patching could be a way forward if we aren't able to
conclude on long-term improvements and cleaning up the mess. My
concern is that a point solution could just add more complexity
without resolving the problems around configuration modularity and
hierarchy.

> Pulsar's configuration is like a trashbox, to which everyone throws
> trash without any consideration. Some configs are hard to understand
> without looking into the implementation details. The PIP requirement
> should prevent such things but it seems that very few contributors
> follow it.

This is the reason why separating implementation-level detail configs
("Private" audience) and user-level configs ("Public" audience) could
be helpful. I guess that a solution in the direction of PIP-346 could
help with this categorization in addition to using annotations in the
ServiceConfiguration file. I wonder if instead of "broker.conf" and
"broker-defaults.conf", there could be two separate files:
"broker.conf" for public settings and "broker-internal.conf" for the
internal implementation-level detail settings. However, that could
introduce new problems such as issues with the
"apply-config-from-env.py" script and Pulsar docker container
configuration.

-Lari

1 - https://lists.apache.org/thread/8splwyrn25pt5rb6ph0yz0tfgknn3hqh
2 - https://en.wikipedia.org/wiki/God_object
3 - https://github.com/apache/pulsar/pull/22274/files


On Wed, 26 Feb 2025 at 09:13, Yunze Xu <xy...@apache.org> wrote:
>
> I opened a PR to clean up some recent config changes:
> https://github.com/apache/pulsar/pull/24027
>
> Pulsar's configuration is like a trashbox, to which everyone throws
> trash without any consideration. Some configs are hard to understand
> without looking into the implementation details. The PIP requirement
> should prevent such things but it seems that very few contributors
> follow it.
>
> I raised my concern two years ago [1] but things are getting worse.
> IMHO, most configs do not have any value to exist. For these configs,
> no one knows when it should be changed and if the customized value
> could be better. AUthors tend to have no responsibility for the config
> added by them. They just think, "you should thank me for providing a
> config to control the behavior". They never stand from a perspective
> of users.
>
> [1] https://lists.apache.org/thread/j23ny19opmp8jww57gwk7g27b5dvl0ot
>
> Thanks,
> Yunze
>
> On Wed, Feb 26, 2025 at 2:33 PM WenZhi Feng <th...@apache.org> wrote:
> >
> > Same confusion about the criterion of PIP.
> >
> > Wenzhi Feng.
> >
> > On 2025/02/26 04:42:27 Yunze Xu wrote:
> > > Hi all,
> > >
> > > I noticed two PRs were merged recently and cherry-picked into
> > > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> > >
> > > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > > implementation of PIP-322. However, it adds a new configuration
> > > without a new PIP or change on the existing PIP. It definitely
> > > violates "When is a PIP required" [1]. (Any change to the
> > > configuration)
> > >
> > > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > > caused by geo-replication by introducing a new feature flag
> > > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > > feature for Pulsar brokers or client)
> > >
> > > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > > 4.0)
> > >
> > > I found the definition of "bug-fixes" ambiguous. It seems like a
> > > silver bullet that if we want a PR to be included in the LTS release,
> > > we only need to say, "it's a bug fix", no matter if it changes
> > > anything that requires a PIP.
> > >
> > > Based on the fact that the PIP rule is not widely followed, should we
> > > update and loose the requirement?
> > >
> > > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> > >
> > > Thanks,
> > > Yunze
> > >

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by Yunze Xu <xy...@apache.org>.
I opened a PR to clean up some recent config changes:
https://github.com/apache/pulsar/pull/24027

Pulsar's configuration is like a trashbox, to which everyone throws
trash without any consideration. Some configs are hard to understand
without looking into the implementation details. The PIP requirement
should prevent such things but it seems that very few contributors
follow it.

I raised my concern two years ago [1] but things are getting worse.
IMHO, most configs do not have any value to exist. For these configs,
no one knows when it should be changed and if the customized value
could be better. AUthors tend to have no responsibility for the config
added by them. They just think, "you should thank me for providing a
config to control the behavior". They never stand from a perspective
of users.

[1] https://lists.apache.org/thread/j23ny19opmp8jww57gwk7g27b5dvl0ot

Thanks,
Yunze

On Wed, Feb 26, 2025 at 2:33 PM WenZhi Feng <th...@apache.org> wrote:
>
> Same confusion about the criterion of PIP.
>
> Wenzhi Feng.
>
> On 2025/02/26 04:42:27 Yunze Xu wrote:
> > Hi all,
> >
> > I noticed two PRs were merged recently and cherry-picked into
> > branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> >
> > https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> > implementation of PIP-322. However, it adds a new configuration
> > without a new PIP or change on the existing PIP. It definitely
> > violates "When is a PIP required" [1]. (Any change to the
> > configuration)
> >
> > https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> > caused by geo-replication by introducing a new feature flag
> > `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> > feature for Pulsar brokers or client)
> >
> > I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> > a new config `managedLedgerOffloadReadThreads` as well. Though it's
> > only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> > 4.0)
> >
> > I found the definition of "bug-fixes" ambiguous. It seems like a
> > silver bullet that if we want a PR to be included in the LTS release,
> > we only need to say, "it's a bug fix", no matter if it changes
> > anything that requires a PIP.
> >
> > Based on the fact that the PIP rule is not widely followed, should we
> > update and loose the requirement?
> >
> > [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> >
> > Thanks,
> > Yunze
> >

Re: [DISCUSS] Do we need to update the documents for PIP and LTS releases?

Posted by WenZhi Feng <th...@apache.org>.
Same confusion about the criterion of PIP.

Wenzhi Feng.

On 2025/02/26 04:42:27 Yunze Xu wrote:
> Hi all,
> 
> I noticed two PRs were merged recently and cherry-picked into
> branch-3.0 and branch-4.0, where 3.0 and 4.0 are LTS releases.
> 
> https://github.com/apache/pulsar/pull/24012 fixes a bug for the
> implementation of PIP-322. However, it adds a new configuration
> without a new PIP or change on the existing PIP. It definitely
> violates "When is a PIP required" [1]. (Any change to the
> configuration)
> 
> https://github.com/apache/pulsar/pull/23697 fixes a message loss issue
> caused by geo-replication by introducing a new feature flag
> `supports_repl_dedup_by_lid_and_eid`. It also violates [1] (Any new
> feature for Pulsar brokers or client)
> 
> I also noticed https://github.com/apache/pulsar/pull/24025, which adds
> a new config `managedLedgerOffloadReadThreads` as well. Though it's
> only cherry-picked into branch-3.3 but not for LTS releases (3.0 and
> 4.0)
> 
> I found the definition of "bug-fixes" ambiguous. It seems like a
> silver bullet that if we want a PR to be included in the LTS release,
> we only need to say, "it's a bug fix", no matter if it changes
> anything that requires a PIP.
> 
> Based on the fact that the PIP rule is not widely followed, should we
> update and loose the requirement?
> 
> [1] https://github.com/apache/pulsar/tree/master/pip#when-is-a-pip-required
> 
> Thanks,
> Yunze
>