You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/04/23 14:09:12 UTC
[GitHub] [rocketmq-mqtt] YxAc opened a new pull request, #82: improving codeCov for #22
YxAc opened a new pull request, #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82
continuously enhancing #22
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] pingww commented on a diff in pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
pingww commented on code in PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#discussion_r857054236
##########
mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java:
##########
@@ -59,22 +59,17 @@ public int nextId(String clientId) {
MsgIdEntry msgIdEntry = hashMsgID(clientId);
synchronized (msgIdEntry) {
int startingMessageId = msgIdEntry.nextMsgId;
- int loopCount = 0;
- int maxLoopCount = 1;
do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
- loopCount++;
- if (loopCount >= maxLoopCount) {
Review Comment:
Yes, it may be the case that maxLoopCount>1 was considered before
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on a diff in pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on code in PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#discussion_r857214091
##########
mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java:
##########
@@ -59,22 +59,17 @@ public int nextId(String clientId) {
MsgIdEntry msgIdEntry = hashMsgID(clientId);
synchronized (msgIdEntry) {
int startingMessageId = msgIdEntry.nextMsgId;
- int loopCount = 0;
- int maxLoopCount = 1;
do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
- loopCount++;
- if (loopCount >= maxLoopCount) {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu merged pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
tianliuliu merged PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] YxAc commented on a diff in pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
YxAc commented on code in PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#discussion_r856926709
##########
mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java:
##########
@@ -59,22 +59,17 @@ public int nextId(String clientId) {
MsgIdEntry msgIdEntry = hashMsgID(clientId);
synchronized (msgIdEntry) {
int startingMessageId = msgIdEntry.nextMsgId;
- int loopCount = 0;
- int maxLoopCount = 1;
do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
- loopCount++;
- if (loopCount >= maxLoopCount) {
Review Comment:
i think this is not necessary, the if judgment will absolutely happen when the 'loopCount' plus
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on a diff in pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on code in PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#discussion_r857214091
##########
mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java:
##########
@@ -59,22 +59,17 @@ public int nextId(String clientId) {
MsgIdEntry msgIdEntry = hashMsgID(clientId);
synchronized (msgIdEntry) {
int startingMessageId = msgIdEntry.nextMsgId;
- int loopCount = 0;
- int maxLoopCount = 1;
do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
- loopCount++;
- if (loopCount >= maxLoopCount) {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] codecov-commenter commented on pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#issuecomment-1107478542
# [Codecov](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#82](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6e54f31) into [main](https://codecov.io/gh/apache/rocketmq-mqtt/commit/344cb70dafd074b60e18e48fe63a436005c0339b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (344cb70) will **increase** coverage by `0.72%`.
> The diff coverage is `100.00%`.
```diff
@@ 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](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...che/rocketmq/mqtt/cs/session/infly/InFlyCache.java](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bXF0dC1jcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbXF0dC9jcy9zZXNzaW9uL2luZmx5L0luRmx5Q2FjaGUuamF2YQ==) | `82.43% <100.00%> (+24.32%)` | :arrow_up: |
| [...ache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bXF0dC1jcy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbXF0dC9jcy9zZXNzaW9uL2luZmx5L01xdHRNc2dJZC5qYXZh) | `96.77% <100.00%> (+42.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [344cb70...6e54f31](https://codecov.io/gh/apache/rocketmq-mqtt/pull/82?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] tianliuliu commented on a diff in pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
tianliuliu commented on code in PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#discussion_r857059220
##########
mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java:
##########
@@ -59,22 +59,17 @@ public int nextId(String clientId) {
MsgIdEntry msgIdEntry = hashMsgID(clientId);
synchronized (msgIdEntry) {
int startingMessageId = msgIdEntry.nextMsgId;
- int loopCount = 0;
- int maxLoopCount = 1;
do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
- loopCount++;
- if (loopCount >= maxLoopCount) {
Review Comment:
in the old code, the var maxLoopCount=2, but it may be cause the getNextId become slow ,then hang the thread
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] YxAc commented on a diff in pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
YxAc commented on code in PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#discussion_r857087707
##########
mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java:
##########
@@ -59,22 +59,17 @@ public int nextId(String clientId) {
MsgIdEntry msgIdEntry = hashMsgID(clientId);
synchronized (msgIdEntry) {
int startingMessageId = msgIdEntry.nextMsgId;
- int loopCount = 0;
- int maxLoopCount = 1;
do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
- loopCount++;
- if (loopCount >= maxLoopCount) {
Review Comment:
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 or no sense to run the loop once or twice.
so may I remove the loop judgment and keep this change? @pingww @tianliuliu
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq-mqtt] YxAc commented on a diff in pull request #82: improving codeCov for #22
Posted by GitBox <gi...@apache.org>.
YxAc commented on code in PR #82:
URL: https://github.com/apache/rocketmq-mqtt/pull/82#discussion_r857087707
##########
mqtt-cs/src/main/java/org/apache/rocketmq/mqtt/cs/session/infly/MqttMsgId.java:
##########
@@ -59,22 +59,17 @@ public int nextId(String clientId) {
MsgIdEntry msgIdEntry = hashMsgID(clientId);
synchronized (msgIdEntry) {
int startingMessageId = msgIdEntry.nextMsgId;
- int loopCount = 0;
- int maxLoopCount = 1;
do {
msgIdEntry.nextMsgId++;
if (msgIdEntry.nextMsgId > MAX_MSG_ID) {
msgIdEntry.nextMsgId = MIN_MSG_ID;
}
if (msgIdEntry.nextMsgId == startingMessageId) {
- loopCount++;
- if (loopCount >= maxLoopCount) {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org