Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[fix][broker] ClassCastException when specify flow_or_qps_equally_div… #17872

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[fix][broker] ClassCastException when specify flow_or_qps_equally_div… #17872

wants to merge 1 commit into from

Conversation

zhangqian321
Copy link
Contributor

@zhangqian321 zhangqian321 commented Sep 28, 2022 •
edited
Loading

splitAlgorithm is alreay specified by user or default before input to this method. It will initial wrong option type and throw ClassCastException when user specified FlowOrQpsEquallyDivideBundleSplitAlgorithm and default is not.

image

@github-actions
Copy link

@zhangqian321 Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

wolfstudy
wolfstudy approved these changes Sep 28, 2022
View reviewed changes
Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1

@wolfstudy wolfstudy added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Sep 28, 2022
@wolfstudy wolfstudy added this to the 2.11.0 milestone Sep 28, 2022
@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Sep 28, 2022
HQebupt
HQebupt reviewed Sep 28, 2022
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
@@ -832,8 +833,7 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
NamespaceBundleSplitAlgorithm splitAlgorithm,
List<Long> boundaries) {
BundleSplitOption bundleSplitOption;
if (config.getDefaultNamespaceBundleSplitAlgorithm()
.equals(NamespaceBundleSplitAlgorithm.FLOW_OR_QPS_EQUALLY_DIVIDE)) {
if (splitAlgorithm instanceof FlowOrQpsEquallyDivideBundleSplitAlgorithm) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Left a minor suggestion. Please add a test to avoid regression.

lordcheng10
lordcheng10 approved these changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@lordcheng10 lordcheng10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

aloyszhang
aloyszhang approved these changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@aloyszhang aloyszhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Jason918
Jason918 approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@Jason918 Jason918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

codelipenghui
codelipenghui reviewed Oct 13, 2022
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zhangqian321 Nice catch!

Could you please add an unit test?

@codelipenghui codelipenghui added ready-to-test type/bug The PR fixed a bug or issue reported a bug area/broker labels Oct 13, 2022
@codelipenghui codelipenghui reopened this Oct 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@0678b82). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #17872   +/-   ##
=========================================
  Coverage          ?   45.70%           
  Complexity        ?    17526           
=========================================
  Files             ?     1573           
  Lines             ?   128208           
  Branches          ?    14097           
=========================================
  Hits              ?    58597           
  Misses            ?    63557           
  Partials          ?     6054
Flag Coverage Δ
unittests 45.70% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

@rdhabalia
Copy link
Contributor

how can we miss such ClassCastException in the Pulsar code? didn't we add any unit-test case for the PR which caused this failure? certainly, not good practice in code have such RuntImeException.
I am seeing a lot of such instances lately in Pulsar which should be avoided. Because of such practice, it takes a lot of minor releases to have a stable release.
I am seeing a lot of necessary PRs without reviews and merging such PRs which keeps creating instability in code.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Nov 13, 2022
@RobertIndie RobertIndie modified the milestones: 2.11.0, 3.1.0 Apr 11, 2023
@Technoboy- Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@lhotari lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui left review comments

@HQebupt HQebupt HQebupt left review comments

@aloyszhang aloyszhang aloyszhang approved these changes

@wolfstudy wolfstudy wolfstudy approved these changes

@lordcheng10 lordcheng10 lordcheng10 approved these changes

@Jason918 Jason918 Jason918 approved these changes

@eolivelli eolivelli Awaiting requested review from eolivelli

@congbobo184 congbobo184 Awaiting requested review from congbobo184

@315157973 315157973 Awaiting requested review from 315157973

Assignees

@zhangqian321 zhangqian321

Labels
area/broker doc-label-missing ready-to-test release/4.0.2 Stale type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.