On Mon, 28 Jul 2025 at 23:30, PengHui Li <[email protected]> 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
