-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Issue 14013][broker] make getInternalStats method async #14047
Conversation
257553b
to
d596fab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1.
but I have two suggestions:
Remove asyncResponse from the internal method, and unified exception and result handling are performed at the controller layer, which makes the internal method purer.
Have you checked to see if there is a test for the modified method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Please fix test failure.
/pulsarbot rerun-failure-checks |
The pr had no activity for 30 days, mark with Stale label. |
@HQebupt Please resolve the test failure. |
/pulsarbot rerun-failure-checks |
d596fab
to
c75cc08
Compare
/pulsarbot rerun-failure-checks |
Please don't rerun failed checks. That won't make the tests pass since this PR contains outdated GitHub Actions workflow changes. You must rebase this PR, push new commits or merge master branch changes to it. Please check the dev mailing list for more information. |
Got it. Thx Lari @lhotari |
c75cc08
to
971e4a4
Compare
971e4a4
to
59ec08b
Compare
59ec08b
to
b2ab007
Compare
The pr had no activity for 30 days, mark with Stale label. |
The pr had no activity for 30 days, mark with Stale label. |
Master Issue: #14013
Motivation
See #14013
Verifying this change
Make sure that the change passes the CI checks.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
no-need-doc