Hi Lari, Thank you for your work but I have a simple question about why we should rush for specific PR and why it's necessary to keep a particular PR part of ongoing release. I know multiple PRs that tried to address critical bugs on this similar issue and they were again blocked without solid reasons and I still have a few concerns about this approach but I don't want to block this proposal as this is addressing a long list of issues that can be useful for key-shared sub, But if we rush and release a new Pulsar release soon with these changes then there is a high chance of regression bugs in the release.
I know we should actively work to address such critical issues because I know many other critical PRs that are solving fundamental and performance problems are just sitting for years because they were blocked due to unknown/personal reasons by reviewers, But I also wish to have this critical PRs in the master branch for testing before releasing a new Pulsar-Release because I am not sure if it's worth to release in rush at the cost of unstable release. Thanks, Rajan On Fri, Oct 4, 2024 at 3:21 PM Lari Hotari <lhot...@apache.org> wrote: > This is a friendly reminder about the urgent need for review of PR #23352, > the first part of implementing "PIP-379: Key_Shared Draining Hashes for > Improved Message Ordering". > > PR: https://github.com/apache/pulsar/pull/23352 > Target: 4.0.0-preview.2 release, scheduled for Monday Oct 7th. > > Your prompt review would be greatly appreciated, ideally by end of day > today. This will allow time for addressing feedback over the weekend. > Thank you for your attention to this matter. > > -Lari > > On 2024/10/02 21:43:53 Lari Hotari wrote: > > Dear Pulsar Community, > > > > I am excited to announce that the first PR implementing "PIP-379: > > Key_Shared Draining Hashes for Improved Message Ordering" is now ready > > for review. This PR contains the core implementation as described in > > the PIP-379 document. PIP-379 is currently in voting. > > > > Since there is a goal to include this implementation in the > > 4.0.0-preview.2 release of 4.0.0, there is urgency to get the review > > started so that we can complete it by EOD on Monday, October 7th. I > > hope to receive review feedback as soon as possible so that I will > > have time to address the review comments. > > > > PR for review: https://github.com/apache/pulsar/pull/23352 > > Please read the PIP-379 document to get the context. The link is in > > the PR description. > > > > Some notable details: > > > > 1. The implementation focuses on maintaining the key invariant for > > Key_Shared subscriptions: "Messages with the same key are delivered > > and allowed to be in an unacknowledged state to only one consumer at a > > time." > > > > 2. More tests will be added to cover the new logic in classes such as > > DrainingHashesTracker, HashRanges (utility class), and PendingAcksMap. > > These tests will be added to the current PR asap. > > > > 3. Observability features will be implemented in a subsequent PR after > > this one is completed. > > > > 4. The vote for PIP-379 is currently open at: > > https://lists.apache.org/thread/z1kgo34qfkkvdnn3l007bdvjr3qqf4rw > > > > Important implementation notes: > > > > - While working on this solution, I discovered a potential memory leak > > in the current pending ack implementations. I do not think that the > > current implementation in Pulsar contains a solution for removing > > pending acks if the mark delete position moves forward as a result of > > cursor reset (or any other reason where it moves forward without > > individual acks). The PIP-379 implementation addresses this issue in > > the PendingAcksMap.removeAllUpTo method, which gets called from the > > PersistentDispatcherMultipleConsumers.readMoreEntries method. > > > > - Significant improvements have been made to ensure the consistency of > > pending acks, which is crucial for maintaining the Key_Shared > > subscription invariant. > > > > I kindly request the community to review the first implementation PR: > > https://github.com/apache/pulsar/pull/23352. Your feedback and > > suggestions are crucial for ensuring the quality and correctness of > > this important improvement to Pulsar's Key_Shared subscriptions. > > > > Thank you for your time and feedback, > > > > Lari > > >
