I think my initial question was quite straightforward: Can we allow more time for reviewers to go through critical changes after a proposal is aligned? A few hours are likely not enough, especially considering reviewers may be in different time zones.
2 cents about the configuration: This isn’t just about the ability to roll back to an old version. Rollbacks are not always straightforward—particularly for large organizations. Users should be able to assess the major changes in a new version, weigh the potential risks, and decide whether or not to adopt the improvements. If they consider the risks too high or the improvements non-essential, they should have the option to disable them entirely. When it comes to critical paths, risk management should take clear precedence over other considerations like maintenance or code style. Maybe only 5% of users will benefit from the improvement, but 100% will be exposed to the risk if it’s not handled carefully. We can’t eliminate risk entirely, but for critical areas, we should design conservatively and do everything possible to ensure a smooth and safe rollout. - Penghui On Mon, Jul 28, 2025 at 2:58 PM Lari Hotari <l...@hotari.net> wrote: > On Mon, 28 Jul 2025 at 23:30, PengHui Li <peng...@apache.org> wrote: > > > > > The PR https://github.com/apache/pulsar/pull/24363 has been in review > > since May 29th. It was simply waiting for PIP-430 to pass. > > There have been plenty of opportunities for reviews before these > > changes have been merged. > > > > But other people are going to review the details after the proposal is > > approved. > > Why we do a detailed code review before aligned with the proposal? > > It would be very great to continue to get reviews and feedback even > after a proposal has been approved. > Improving the design of PIPs continues to happen in Pulsar even after > we have approved PIPs. > > There wouldn't be a benefit to keep on getting into very low level > details before a PIP is approved. > In the Pulsar Community, we have set certains goals which are > documented at > https://github.com/apache/pulsar/tree/master/pip#what-is-the-goal-of-a-pip > . > The last sentence is "It is not a goal for PIP to add undue process or > slow-down the development". > There has never been a requirement in our PIP process that all > implementation level details are fully aligned before proceeding to > approval. > > > > I think that PR has been adequately reviewed by Matteo. If you have > > concerns about this work, please let me know. > > > > I'm not concerned about one person's review quality. I'm concerned about > > the PR gets merged with only 1 approval for such core cache changes in > > Pulsar. > > > > > In PIP-430, there's already configuration options for > > selecting the way how caching works. > > I don't currently think that other configuration options than what > > PIP-430 has would be needed in practice. > > > > Can you explain more about how the user can continue to use the > > existing cache implementation? > > I checked > > > https://github.com/lhotari/pulsar/blob/lh-pip-430/pip/pip-430.md#configuration > > Where mentioned that there is a configuration to disable the new > solution? > > Pulsar users shouldn't need to care what implementation is used exactly. > In the case of PIP-430, the configurable parameters are about keeping > the observable behavior of the cache more or less unchanged. > By setting `cacheEvictionByExpectedReadCount ` to `false`, the > observable behavior will be sufficiently similar as without PIP-430 > changes. > > There is no `managedLedgerCacheManagerImplementationClass` config > parameter in PIP-430. There are reasons why the old cache > implementation shouldn't be kept around as-is. One of them is the need > to change the current abstraction EntryCacheManager/EntryCache > slightly. It is easier to handle by allowing the > `EntryCacheManager`/`EntryCache` interfaces to change until the goals > of PIP-430 have been reached. > > Since the current PIP-430 implementation is a refactoring of the > current implementation, it doesn't make sense to start the > implementation from scratch. It would be possible to return the old > implementation from the state before making the changes within a > different package. However, keeping the old implementation around > as-is is also a challenge from the perspective of maintenance. There > would be a need to cover testing for 2 implementations instead of 1. > All of this would add overhead without much added value. Nothing > prevents doing this later and I think it's a lot easier to add the old > implementation back later if we'd decide that it's really essential. > Since there are differences in the abstractions regarding evictions, > it would also require some adaptation to keep the old implementation > around so this wouldn't be great from a code maintenance perspective > since exactly the same legacy implementation cannot be used. > > I understand that you are concerned that we could face a problem in > production after starting to use the new solution. > If that were to happen, we can deal with it as any other bug that we > deal with. In most cases, a deployment should be rolled back to the > previous version until a fix is made available. > > -Lari >
