> 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? > 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? - Penghui On Mon, Jul 28, 2025 at 12:43 PM Lari Hotari <[email protected]> wrote: > On Mon, 28 Jul 2025 at 22:06, PengHui Li <[email protected]> wrote: > > > > Why was there such urgency in merging a proposal and implementation that > > introduces significant changes to the hot path? > > 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. > My motivation for merging PR 24363 is that I'd like to rebase the > changes I have for the next part of the PIP-430 implementation. > Please notice that these changes are going to the master branch and > won't impact released versions of Pulsar. > > > Both the proposal and implementation PR received just one approval, > despite > > containing a substantial number of changes: +2,315 / −1,450 lines. Since > > that initial approval, 11 additional commits have been pushed to the PR. > > > > Who reviewed these 11 commits? > > Were they adequately reviewed given the scope and impact of the changes? > > These are the 11 commits that happened after Matteo's review: > > https://github.com/lhotari/pulsar/compare/365fc07f727044a614fc35d43f6b5f6b6a157108...b62ccdddbf9b787b0b1e9e584f26e57f00469f33 > These aren't substancial changes. They are addressing Matteo's review > comments and fixing test failures. > I think that PR has been adequately reviewed by Matteo. If you have > concerns about this work, please let me know. > When something gets merged, it doesn't mean that we have set things in > stone at that point. > > > > Thank you, Penghui. This is a vote thread, so we can handle this > concern > > separately once the work proceeds in implementation PRs. > > > > Ok, my bad. I will give my explicit -1 next time. The proposal needs to > add > > the configuration change, no? > > Sure, it will be helpful to raise feedback during the discussion phase > of a PIP. 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. > > > Please take > > > https://github.com/apache/pulsar/pull/15955/files#diff-1e63fb74fd8bcabf5320f10d72ea893e1ce72cbb5b7f55a49460674d905c09c7R1063 > > for example. > > Yes, that solution has been considered while designing PIP-430. > > -Lari >
