Skip to content

Conversation

nowinkeyy
Copy link
Contributor

@nowinkeyy nowinkeyy commented Sep 26, 2022

Make sure set the target branch to develop

What is the purpose of the change

The first PR #5166
fix #5122 for rocketmq-broker module

Brief changelog

XX

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Merging #5193 (531aa0b) into develop (b8607d9) will increase coverage by 0.07%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             develop    #5193      +/-   ##
=============================================
+ Coverage      43.19%   43.27%   +0.07%     
- Complexity      7795     7813      +18     
=============================================
  Files            998      998              
  Lines          69456    69456              
  Branches        9174     9174              
=============================================
+ Hits           30002    30054      +52     
+ Misses         35671    35629      -42     
+ Partials        3783     3773      -10     
Impacted Files Coverage Δ
...org/apache/rocketmq/store/ha/WaitNotifyObject.java 80.35% <0.00%> (-5.36%) ⬇️
...che/rocketmq/broker/filter/ConsumerFilterData.java 94.59% <0.00%> (-2.71%) ⬇️
...mq/store/ha/autoswitch/AutoSwitchHAConnection.java 70.43% <0.00%> (-1.08%) ⬇️
...ent/impl/consumer/DefaultLitePullConsumerImpl.java 68.75% <0.00%> (-0.47%) ⬇️
...he/rocketmq/client/impl/consumer/ProcessQueue.java 61.46% <0.00%> (-0.46%) ⬇️
...rocketmq/client/impl/factory/MQClientInstance.java 46.82% <0.00%> (+0.14%) ⬆️
...main/java/org/apache/rocketmq/store/CommitLog.java 68.69% <0.00%> (+0.18%) ⬆️
...apache/rocketmq/store/timer/TimerMessageStore.java 54.18% <0.00%> (+0.19%) ⬆️
.../apache/rocketmq/logging/inner/LoggingBuilder.java 63.92% <0.00%> (+0.31%) ⬆️
...lient/impl/consumer/DefaultMQPushConsumerImpl.java 35.40% <0.00%> (+0.38%) ⬆️
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nowinkeyy nowinkeyy requested a review from lizhanhui September 27, 2022 06:30
@aaron-ai
Copy link
Member

aaron-ai commented Sep 27, 2022

@nowinkeyy Could you rebase to the latest develop branch and force-push to your remote branch? It seems that irrelevant test don't pass.

@nowinkeyy
Copy link
Contributor Author

Hi @lizhanhui. I'm sorry i hit the wrong button, and now it seems to need your review.

Copy link
Contributor

@tsunghanjacktsai tsunghanjacktsai left a comment

Choose a reason for hiding this comment

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

Hi @nowinkeyy ,

Great improvement. However, I'm a bit confused with some changes in your PR.

.asf.yaml Outdated
@@ -27,7 +27,7 @@ github:
# Enable squash button
squash: true
# Disable merge button
merge: true
merge: false
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of modifying this parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks some commits from develop branch are merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this submission is the result of rebase the develop branch.

README.md Outdated
[![Percentage of issues still open](http://isitmaintained.com/badge/open/apache/rocketmq.svg)](http://isitmaintained.com/project/apache/rocketmq "Percentage of issues still open")
[![Twitter Follow](https://img.shields.io/twitter/follow/ApacheRocketMQ?style=social)](https://twitter.com/intent/follow?screen_name=ApacheRocketMQ)
## Apache RocketMQ

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the change relevant to the main purpose of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should have been corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

@lizhanhui
Copy link
Contributor

@nowinkeyy

I'm sorry i hit the wrong button, and now it seems to need your review.

It's OK. Let me help you fix the CI issue.

@lizhanhui lizhanhui merged commit d52be62 into apache:develop Sep 28, 2022
@nowinkeyy nowinkeyy deleted the rocketmq-5122-broker branch September 28, 2022 04:10
drpmma pushed a commit that referenced this pull request Feb 21, 2023
#5193)

* style(pom):Enable checkstyle for test code

* style(broker):Enable checkstyle for test code

* Update pom.xml

* Enable checkstyle for tests of broker module

* Remove delete tmp file assertion as OS may have cron job to clean them

* Fix bazel issue

Co-authored-by: Aaron Ai <[email protected]>
Co-authored-by: Li Zhanhui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable checkstyle for test code
6 participants