[improve][broker]Delete unnecessary calls #24045
Conversation
@guan46 Please explain what "improve performance" means. This type of PRs will be wasting reviewer's time when there isn't sufficient rationale. Instead of being helpful they become harmful. Generative AI could produce a huge amount of PRs for the Pulsar code base and put reviewers to train bots. That has happened in other OSS projects and we don't want that to happen in Apache Pulsar. Please pay more attention when you create new PRs. Adding a name to your profile could also help that we don't mix you into a bot account. Bot accounts will be blocked and reported. |
|
I've been learning about Pulsar recently. I learn it by reading PRs. If I find any issues, I'll also submit PRs myself. I'm not an AI. @lhotari |
Thanks for your clarification, @guan46. Some Apache projects have been receiving PRs which are AI generated with human assistance and I wanted to check that. I hope you understand that's the reason why there's a pushback for anonymous contributors. There's 2 concerns why projects avoid anonymous contributions: the trend of AI training by creating PRs and security. We also want to avoid security incidents where a contributor gains trust and later pushes malicious PRs that get accepted into code bases. The xz/liblzma backdoor incident is a sobering reminder of how anonymous trusted contributors can compromise critical infrastructure. We can put security concerns aside. Since your profile was anonymous it was a red flag to me mainly related to the "AI training" scams that have been hitting OSS repositories. A real person opens the PR, but requested by a AI training company and the review feedback is fed back to train AI and to make follow up changes assisted by AI. For this specific PR, it would be helpful if you could explain in more detail what performance improvements you expect from removing this call, perhaps with some measurements or profiling data that shows the impact. When you provide this clarification, reviewers won't have to spend their limited time in finding out these details. For microbenchmarks we have the microbench module in Pulsar where new JMH microbenchmarks can be added. I'm not saying that it's necessary in this case if there's another way to explain what "improve performance" means and what the expected impact of the change is. |
|
In the individualAckNormal method, the checkCanRemovePendingAcksAndHandle method is called. (The checkCanRemovePendingAcksAndHandle method removes the pending acknowledgments and then updates the blocked state of the consumer.) After subsequently updating the number of unacknowledged messages, the updateBlockedConsumerOnUnackedMsgs method is called again to update the blocked state of the consumer. This is a redundant call. @lhotari |
This PR would introduce a bug in since not all locations of A recent change in this area was #23796 by @summeriiii. This information could be useful for other reviewers. |
|
The unit test has passed. Please take a look. @lhotari |
Which question are you referring to that has not been answered @lhotari |
@guan46 please read #24045 (comment) carefully. It would be great to find more useful contribution targets than eliminating unnecessary method calls for code locations which aren't hotspots confirmed with profiling. You can join the dev channel on Pulsar slack to find more valuable PR subjects from the Pulsar project's point of view. |
Has my PR made Pulsar better? For new members of the community, I believe that what's more important is to offer encouragement. I will grow step by step and make contributions to the community. @lhotari |
@guan46 This particular PR might introduce a bug, I mentioned that in the previous comment: "This PR would introduce a bug in since not all locations of removePendingAcks are handled in this PR." ... "It's possible that tests don't catch the problem that this PR in it's current form would introduce." . We don't have 100% test coverage and that's why doing this type of PRs is not a good way to start contributing. It's better to address real issues that you notice when you use Pulsar, for example going through the user guide and noticing usability issues, inconsistency and bugs. There are plenty of issues that you will spot that way. For example, the Apache Pulsar Helm chart instructions aren't up-to-date in the user guide and there's a similar issue with docker and docker compose instructions that users will run into issues following the user guide. I'd prefer PRs that are addressing issues that you run into while using Pulsar yourself or within your company. Those improvements will surely be valuable for everyone. |
Motivation
There are unnecessary calls in the individualAckNormal. Delete the line of code that calls updateBlockedConsumerOnUnackedMsgs to improve performance.
Modifications
Refer to the Motivation section.
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository:guan46#8