[improve][broker] Refactor a private method to eliminate an unnecessary parameter #23904
Conversation
.../apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java
Outdated
Show resolved
Hide resolved
lhotari
changed the title
| boolean canDispatchEntry = canDispatchEntry(consumer, entry, readType, stickyKeyHash); | ||
| if (!canDispatchEntry) { | ||
| if(blockedByHash != null){ | ||
| blockedByHash.setTrue(); | ||
| } | ||
| // decrement the permits for the consumer | ||
| permits.decrement(); | ||
| // allow the entry to be dispatched | ||
| dispatchEntry = true; | ||
| } |
There was a problem hiding this comment.
This logic is not correct. Do you spot the problem?
There was a problem hiding this comment.
Could you explain it in detail? @lhotari
There was a problem hiding this comment.
For effective contributions, it is necessary to run tests and use tests to provide feedback. Did you check my previous advice about Personal CI? When you know what tests fail, you can then locally run a single unit test in your IDE. Have you already tried this?
There was a problem hiding this comment.
It's been executed. Please take a look @lhotari
There was a problem hiding this comment.
@guan46 Thanks.
For some reason, the "Pulsar CI" workflow didn't run for the pull request. I also noticed that your fork's master branch isn't completely in sync with apache/pulsar master. It's necessary to get a recent CI fix included when you run "Pulsar CI" workflow in your fork. However that's not the reason why the workflow didn't execute.
When you go to https://github.com/guan46/pulsar/actions/workflows/pulsar-ci.yaml , do you see whether the workflow is enabled or not?
Click "Enable workflow" if that is visible.
There was a problem hiding this comment.
In order to sync your forks master branch with apache/pulsar's master, there are some instructions here: https://pulsar.apache.org/contribute/personal-ci/#stay-in-sync-with-upstream
There was a problem hiding this comment.
Could you explain it in detail? @lhotari
Here's Deepseek's detailed explanation based on https://github.com/apache/pulsar/pull/23904.patch
There was a problem hiding this comment.
Please follow instructions to first get a CI pass with "Personal CI" in your forked repository: https://pulsar.apache.org/contribute/personal-ci/ . You will be able to get automated CI feedback on your own without requiring assistance from Pulsar contributors. It's necessary to latest changes from master branch to get most recent CI fixes. We have a lot of flaky tests, so some test failures could be flaky ones. Rerun failed builds in GitHub Actions CI if this happens. In your personal CI you will have full control.
Motivation
Modifications
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: guan46#1