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