Skip to content

make setMessageRoutingMode method more readable and optimize unit test for messageRoutingMode#14726

Closed
yws-tracy wants to merge 4 commits intoapache:masterfrom
yws-tracy:optimize_setMessageRoutingMode
Closed

make setMessageRoutingMode method more readable and optimize unit test for messageRoutingMode#14726
yws-tracy wants to merge 4 commits intoapache:masterfrom
yws-tracy:optimize_setMessageRoutingMode

Conversation

@yws-tracy
Copy link
Contributor

@yws-tracy yws-tracy commented Mar 17, 2022

Motivation

when I first read setMessageRoutingMode, I was confused, the logic is not very clear, and the error hints is not completely right, I try to make the logic more readable and clear, and split into different error hints

hope to be adopted, thanks

Modifications

split the check logic in setMessageRoutingMode method of class ProducerBuilderImpl

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@github-actions
Copy link

@yws-tracy:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

@yws-tracy:Thanks for providing doc info!

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 17, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Mar 17, 2022
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.

It‘s better to provide a unit test to make sure the logic stays the same.

@yws-tracy
Copy link
Contributor Author

unit

It‘s better to provide a unit test to make sure the logic stays the same.

there is enough tests about messageRoutingMode, and I supplement the related unit test to ensure the logic is same

@yws-tracy yws-tracy requested a review from Jason918 March 20, 2022 14:29
@yws-tracy
Copy link
Contributor Author

yws-tracy commented Mar 24, 2022

why cicd build failed? weird..... build ok in local env, can't it retry to run cicd?

@yws-tracy yws-tracy changed the title make setMessageRoutingMode method more readable make setMessageRoutingMode method more readable and supplement unit test for messageRoutingMode Mar 24, 2022
@Jason918
Copy link
Contributor

why cicd build failed? weird..... build ok in local env, can't it retry to run cicd?

You need rebase on latest master.

@yws-tracy yws-tracy force-pushed the optimize_setMessageRoutingMode branch from beecc96 to 555dcc1 Compare March 25, 2022 08:32
@yws-tracy yws-tracy requested a review from RobertIndie March 25, 2022 08:34
@yws-tracy yws-tracy changed the title make setMessageRoutingMode method more readable and supplement unit test for messageRoutingMode make setMessageRoutingMode method more readable and optimize unit test for messageRoutingMode Mar 28, 2022
@yws-tracy
Copy link
Contributor Author

why cicd build failed? weird..... build ok in local env, can't it retry to run cicd?

You need rebase on latest master.

could you help to review again ? thanks a lot

@github-actions
Copy link

github-actions bot commented May 1, 2022

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

@github-actions
Copy link

github-actions bot commented Jun 1, 2022

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

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@tisonkun
Copy link
Member

Closed as stale and conflict. Please rebase and resubmit the patch if it's still relevant.

@tisonkun tisonkun closed this Dec 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs lifecycle/stale Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants