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.

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

improving codeCov for #22 #82

Merged
merged 1 commit into from
Apr 26, 2022
Merged

improving codeCov for #22 #82

merged 1 commit into from
Apr 26, 2022

Conversation

YxAc
Copy link
Contributor

@YxAc YxAc commented Apr 23, 2022

continuously enhancing #22

@codecov-commenter
Copy link

Codecov Report

Merging #82 (6e54f31) into main (344cb70) will increase coverage by 0.72%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   30.35%   31.07%   +0.72%     
==========================================
  Files          97       97              
  Lines        4191     4187       -4     
  Branches      661      660       -1     
==========================================
+ Hits         1272     1301      +29     
+ Misses       2676     2645      -31     
+ Partials      243      241       -2     
Impacted Files Coverage Δ
...che/rocketmq/mqtt/cs/session/infly/InFlyCache.java 82.43% <100.00%> (+24.32%) ⬆️
...ache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java 96.77% <100.00%> (+42.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 344cb70...6e54f31. Read the comment docs.

do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
loopCount++;
if (loopCount >= maxLoopCount) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this is not necessary, the if judgment will absolutely happen when the 'loopCount' plus

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it may be the case that maxLoopCount>1 was considered before

Copy link
Contributor

Choose a reason for hiding this comment

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

in the old code, the var maxLoopCount=2, but it may be cause the getNextId become slow ,then hang the thread

Copy link
Contributor Author

@YxAc YxAc Apr 24, 2022

Choose a reason for hiding this comment

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

Under most cases, the method 'releaseId' which release 'inUseMsgIds' is enough, unless lots of message accumulation happens. Once the extreme case occurs, i think it makes no difference/sense to run the loop once or twice.

so may I remove the loop judgment and keep this change? @pingww @tianliuliu

Copy link
Contributor

@tianliuliu tianliuliu Apr 25, 2022

Choose a reason for hiding this comment

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

Under most cases, the method 'releaseId' which release 'inUseMsgIds' is enough, unless lots of message accumulation happens. Once the extreme case occurs, i think it makes no difference/sense to run the loop once or twice.

so may I remove the loop judgment and keep this change? @pingww @tianliuliu

you are right. the cause is exactly it.

@tianliuliu tianliuliu merged commit 011fbb3 into apache:main Apr 26, 2022
@YxAc YxAc deleted the continuously_improve_codeCov branch April 27, 2022 02:51
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.

4 participants