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/04 08:02:00 UTC
[GitHub] [rocketmq] dugenkui03 opened a new pull request, #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`
dugenkui03 opened a new pull request, #4113:
URL: https://github.com/apache/rocketmq/pull/4113
**Make sure set the target branch to `develop`**
## What is the purpose of the change
#4112
## 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`.
- [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) 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.
- [x] 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.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] 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](https://github.com/apache/rocketmq/tree/master/test).
- [x] 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](http://www.apache.org/licenses/#clas).
--
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] lwclover commented on a diff in pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
lwclover commented on code in PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#discussion_r844564354
##########
client/src/main/java/org/apache/rocketmq/client/impl/consumer/ProcessQueue.java:
##########
@@ -404,30 +404,33 @@ public void incTryUnlockTimes() {
public void fillProcessQueueInfo(final ProcessQueueInfo info) {
try {
this.treeMapLock.readLock().lockInterruptibly();
+ try {
+ if (!this.msgTreeMap.isEmpty()) {
+ info.setCachedMsgMinOffset(this.msgTreeMap.firstKey());
+ info.setCachedMsgMaxOffset(this.msgTreeMap.lastKey());
+ info.setCachedMsgCount(this.msgTreeMap.size());
+ info.setCachedMsgSizeInMiB((int) (this.msgSize.get() / (1024 * 1024)));
+ }
- if (!this.msgTreeMap.isEmpty()) {
- info.setCachedMsgMinOffset(this.msgTreeMap.firstKey());
- info.setCachedMsgMaxOffset(this.msgTreeMap.lastKey());
- info.setCachedMsgCount(this.msgTreeMap.size());
- info.setCachedMsgSizeInMiB((int) (this.msgSize.get() / (1024 * 1024)));
- }
-
- if (!this.consumingMsgOrderlyTreeMap.isEmpty()) {
- info.setTransactionMsgMinOffset(this.consumingMsgOrderlyTreeMap.firstKey());
- info.setTransactionMsgMaxOffset(this.consumingMsgOrderlyTreeMap.lastKey());
- info.setTransactionMsgCount(this.consumingMsgOrderlyTreeMap.size());
- }
+ if (!this.consumingMsgOrderlyTreeMap.isEmpty()) {
+ info.setTransactionMsgMinOffset(this.consumingMsgOrderlyTreeMap.firstKey());
+ info.setTransactionMsgMaxOffset(this.consumingMsgOrderlyTreeMap.lastKey());
+ info.setTransactionMsgCount(this.consumingMsgOrderlyTreeMap.size());
+ }
- info.setLocked(this.locked);
- info.setTryUnlockTimes(this.tryUnlockTimes.get());
- info.setLastLockTimestamp(this.lastLockTimestamp);
+ info.setLocked(this.locked);
+ info.setTryUnlockTimes(this.tryUnlockTimes.get());
+ info.setLastLockTimestamp(this.lastLockTimestamp);
- info.setDroped(this.dropped);
- info.setLastPullTimestamp(this.lastPullTimestamp);
- info.setLastConsumeTimestamp(this.lastConsumeTimestamp);
- } catch (Exception e) {
- } finally {
- this.treeMapLock.readLock().unlock();
+ info.setDroped(this.dropped);
+ info.setLastPullTimestamp(this.lastPullTimestamp);
+ info.setLastConsumeTimestamp(this.lastConsumeTimestamp);
+ } catch (Exception e) {
+ } finally {
+ this.treeMapLock.readLock().unlock();
+ }
+ } catch (InterruptedException e) {
Review Comment:
Maybe it's better to use Exception
--
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] coveralls commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1091109515
[](https://coveralls.io/builds/48072066)
Coverage increased (+0.04%) to 52.0% when pulling **c99f3a82fa9f3df3d79115a4cab496a8a6a06687 on dugenkui03:patch-10** into **fd554ab12072225325c957ff6bdf492fc67821af on apache:develop**.
--
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] lwclover commented on a diff in pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
lwclover commented on code in PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#discussion_r844565879
##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/RouteInfoManager.java:
##########
@@ -195,11 +194,13 @@ public RegisterBrokerResult registerBroker(
}
}
}
- } finally {
+ } catch (Exception e) {
+ log.error("registerBroker Exception", e);
+ }finally {
this.lock.writeLock().unlock();
}
- } catch (Exception e) {
- log.error("registerBroker Exception", e);
+ } catch (InterruptedException e) {
+ log.error("registerBroker fail due to an InterruptedException");
Review Comment:
I think caused param will be better
--
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] codecov-commenter commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1091109692
# [Codecov](https://codecov.io/gh/apache/rocketmq/pull/4113?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 [#4113](https://codecov.io/gh/apache/rocketmq/pull/4113?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c99f3a8) into [develop](https://codecov.io/gh/apache/rocketmq/commit/fd554ab12072225325c957ff6bdf492fc67821af?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fd554ab) will **decrease** coverage by `0.02%`.
> The diff coverage is `79.31%`.
```diff
@@ Coverage Diff @@
## develop #4113 +/- ##
=============================================
- Coverage 47.92% 47.89% -0.03%
+ Complexity 5002 5001 -1
=============================================
Files 634 635 +1
Lines 42529 42506 -23
Branches 5573 5567 -6
=============================================
- Hits 20381 20360 -21
- Misses 19647 19653 +6
+ Partials 2501 2493 -8
```
| [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/4113?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/rocketmq/client/impl/consumer/ProcessQueue.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9Qcm9jZXNzUXVldWUuamF2YQ==) | `62.27% <64.70%> (-0.12%)` | :arrow_down: |
| [...e/rocketmq/namesrv/routeinfo/RouteInfoManager.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-bmFtZXNydi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbmFtZXNydi9yb3V0ZWluZm8vUm91dGVJbmZvTWFuYWdlci5qYXZh) | `81.51% <100.00%> (ø)` | |
| [...rocketmq/broker/filtersrv/FilterServerManager.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-YnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9icm9rZXIvZmlsdGVyc3J2L0ZpbHRlclNlcnZlck1hbmFnZXIuamF2YQ==) | `20.00% <0.00%> (-14.29%)` | :arrow_down: |
| [...in/java/org/apache/rocketmq/test/util/MQAdmin.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-dGVzdC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvdGVzdC91dGlsL01RQWRtaW4uamF2YQ==) | `38.88% <0.00%> (-5.56%)` | :arrow_down: |
| [...ava/org/apache/rocketmq/test/util/VerifyUtils.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-dGVzdC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvdGVzdC91dGlsL1ZlcmlmeVV0aWxzLmphdmE=) | `46.26% <0.00%> (-2.99%)` | :arrow_down: |
| [...ava/org/apache/rocketmq/filter/util/BitsArray.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-ZmlsdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9maWx0ZXIvdXRpbC9CaXRzQXJyYXkuamF2YQ==) | `59.82% <0.00%> (-2.57%)` | :arrow_down: |
| [...va/org/apache/rocketmq/logging/inner/Appender.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9BcHBlbmRlci5qYXZh) | `34.83% <0.00%> (-2.25%)` | :arrow_down: |
| [...ent/impl/consumer/DefaultLitePullConsumerImpl.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9EZWZhdWx0TGl0ZVB1bGxDb25zdW1lckltcGwuamF2YQ==) | `67.75% <0.00%> (-1.56%)` | :arrow_down: |
| [...rg/apache/rocketmq/remoting/netty/NettyLogger.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-cmVtb3Rpbmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3JlbW90aW5nL25ldHR5L05ldHR5TG9nZ2VyLmphdmE=) | `14.96% <0.00%> (-1.37%)` | :arrow_down: |
| [...a/org/apache/rocketmq/store/StoreStatsService.java](https://codecov.io/gh/apache/rocketmq/pull/4113/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL1N0b3JlU3RhdHNTZXJ2aWNlLmphdmE=) | `35.85% <0.00%> (-1.13%)` | :arrow_down: |
| ... and [17 more](https://codecov.io/gh/apache/rocketmq/pull/4113/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/rocketmq/pull/4113?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/pull/4113?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 [fd554ab...c99f3a8](https://codecov.io/gh/apache/rocketmq/pull/4113?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] dugenkui03 commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1091087578
@lwclover
#### Thanks for your reivew and suggestion
`RouteInfoManager` log is modified for doing the same logic with others.
The main propurse of this pr is that fixing the error rised from the incorrect position of `lockInterruptibly`, and not change the semantics of any method.
#### 感谢建议和评论。
1. `RouteInfoManager` 中的日志打印已加上异常堆栈、**以和其他处理`InterruptedException`的逻辑对齐**。但其实这是没有必要的，因为加锁方法抛出的`InterruptedException`并不会打印任何有用信息、并且抛出异常的位置也是唯一的；
2. 其他建议均为同一类型：修改catch的异常类型、或者不catch某些异常。**这种修改会改变所在方法的语义**，比如之前方法的语义仅仅是 catch `InterruptedException`、修改会为catch所有异常、甚至包括 `RuntimeException`，这不是该PR的本意。该PR目的只有一个：修正 `lockInterruptibly` 使用位置导致问题导致的异常准确性和bug。
--
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] lwclover commented on a diff in pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
lwclover commented on code in PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#discussion_r844565515
##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/RouteInfoManager.java:
##########
@@ -195,11 +194,13 @@ public RegisterBrokerResult registerBroker(
}
}
}
- } finally {
+ } catch (Exception e) {
Review Comment:
I think the line ‘catch (Exception e) ’should be removed
--
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] dugenkui03 commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1094171342
@RongtongJin CI未通过因为单测超时，单测本地执行成功，请问这种情况需要如何处理。
单测代码：
```java
boolean recvAll = MQWait.waitConsumeAll(30 * 1000, producer.getAllMsgBody(),
consumer1.getListener(), consumer2.getListener(), consumer3.getListener());
assertThat(recvAll).isEqualTo(true);
```
报错
```java
estThreeConsumerAndCrashOne(org.apache.rocketmq.test.client.producer.order.OrderMsgDynamicRebalanceIT)
Time elapsed: 32.405 sec
```
--
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] MatrixHB commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`
Posted by GitBox <gi...@apache.org>.
MatrixHB commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1088364835
The code is OK, but you need to resolve the conflicts.
--
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] coveralls commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
coveralls commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1091109516
[](https://coveralls.io/builds/48072066)
Coverage increased (+0.04%) to 52.0% when pulling **c99f3a82fa9f3df3d79115a4cab496a8a6a06687 on dugenkui03:patch-10** into **fd554ab12072225325c957ff6bdf492fc67821af on apache:develop**.
--
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] dugenkui03 commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1091096898
@lwclover
> 其他建议均为同一类型：修改catch的异常类型、或者不catch某些异常...
刚才我理解成了4条**单独**的建议，其实4条评论是两个完整的修改建议、这样看代码实现的确是更加简练了。感谢建议。
--
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] MatrixHB commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`
Posted by GitBox <gi...@apache.org>.
MatrixHB commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1088511618
> @MatrixHB I create new branch from **master** instead of develop branch, is this right operation?
>
> I plan resolving the conflicts when the ’conflict change‘ merged into master.
You should create new branch from `develop` branch next time.
For this pr, I suggest you can merge `develop` and resolve conflicts in your local branch, and then push it again.
--
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] dugenkui03 commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1088980699
> > @MatrixHB I create new branch from **master** instead of develop branch, is this right operation?
> > I plan resolving the conflicts when the ’conflict change‘ merged into master.
>
> You should create new branch from `develop` branch next time. For this pr, I suggest you can merge `develop` and resolve conflicts in your local branch, and then push it again. `RouteInfoManager` has some changes in develop branch you need to merge.
Thanks.
--
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] lwclover commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
lwclover commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1091160169
> 其实4条评论是两个完整的修改建议、这样看代码实现的确是更加简练了。感谢建议。
客气了，共同进步
--
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] dugenkui03 commented on a diff in pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on code in PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#discussion_r844662663
##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/RouteInfoManager.java:
##########
@@ -195,11 +194,13 @@ public RegisterBrokerResult registerBroker(
}
}
}
- } finally {
+ } catch (Exception e) {
+ log.error("registerBroker Exception", e);
+ }finally {
this.lock.writeLock().unlock();
}
- } catch (Exception e) {
- log.error("registerBroker Exception", e);
+ } catch (InterruptedException e) {
+ log.error("registerBroker fail due to an InterruptedException");
Review Comment:
Thanks for your review.
--
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] lwclover commented on a diff in pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
lwclover commented on code in PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#discussion_r844565601
##########
namesrv/src/main/java/org/apache/rocketmq/namesrv/routeinfo/RouteInfoManager.java:
##########
@@ -195,11 +194,13 @@ public RegisterBrokerResult registerBroker(
}
}
}
- } finally {
+ } catch (Exception e) {
+ log.error("registerBroker Exception", e);
+ }finally {
this.lock.writeLock().unlock();
}
- } catch (Exception e) {
- log.error("registerBroker Exception", e);
+ } catch (InterruptedException e) {
Review Comment:
Maybe it's better to use Exception
--
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] lwclover commented on a diff in pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`【修复`lock.lockInterruptibly()` 使用不当导致的异常准确性和bug】
Posted by GitBox <gi...@apache.org>.
lwclover commented on code in PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#discussion_r844563390
##########
client/src/main/java/org/apache/rocketmq/client/impl/consumer/ProcessQueue.java:
##########
@@ -404,30 +404,33 @@ public void incTryUnlockTimes() {
public void fillProcessQueueInfo(final ProcessQueueInfo info) {
try {
this.treeMapLock.readLock().lockInterruptibly();
+ try {
+ if (!this.msgTreeMap.isEmpty()) {
+ info.setCachedMsgMinOffset(this.msgTreeMap.firstKey());
+ info.setCachedMsgMaxOffset(this.msgTreeMap.lastKey());
+ info.setCachedMsgCount(this.msgTreeMap.size());
+ info.setCachedMsgSizeInMiB((int) (this.msgSize.get() / (1024 * 1024)));
+ }
- if (!this.msgTreeMap.isEmpty()) {
- info.setCachedMsgMinOffset(this.msgTreeMap.firstKey());
- info.setCachedMsgMaxOffset(this.msgTreeMap.lastKey());
- info.setCachedMsgCount(this.msgTreeMap.size());
- info.setCachedMsgSizeInMiB((int) (this.msgSize.get() / (1024 * 1024)));
- }
-
- if (!this.consumingMsgOrderlyTreeMap.isEmpty()) {
- info.setTransactionMsgMinOffset(this.consumingMsgOrderlyTreeMap.firstKey());
- info.setTransactionMsgMaxOffset(this.consumingMsgOrderlyTreeMap.lastKey());
- info.setTransactionMsgCount(this.consumingMsgOrderlyTreeMap.size());
- }
+ if (!this.consumingMsgOrderlyTreeMap.isEmpty()) {
+ info.setTransactionMsgMinOffset(this.consumingMsgOrderlyTreeMap.firstKey());
+ info.setTransactionMsgMaxOffset(this.consumingMsgOrderlyTreeMap.lastKey());
+ info.setTransactionMsgCount(this.consumingMsgOrderlyTreeMap.size());
+ }
- info.setLocked(this.locked);
- info.setTryUnlockTimes(this.tryUnlockTimes.get());
- info.setLastLockTimestamp(this.lastLockTimestamp);
+ info.setLocked(this.locked);
+ info.setTryUnlockTimes(this.tryUnlockTimes.get());
+ info.setLastLockTimestamp(this.lastLockTimestamp);
- info.setDroped(this.dropped);
- info.setLastPullTimestamp(this.lastPullTimestamp);
- info.setLastConsumeTimestamp(this.lastConsumeTimestamp);
- } catch (Exception e) {
- } finally {
- this.treeMapLock.readLock().unlock();
+ info.setDroped(this.dropped);
+ info.setLastPullTimestamp(this.lastPullTimestamp);
+ info.setLastConsumeTimestamp(this.lastConsumeTimestamp);
+ } catch (Exception e) {
Review Comment:
I think the line ‘catch (Exception e) ’should be removed
--
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] dugenkui03 commented on pull request #4113: [ISSUE #4112]Fix the usage of `lockInterruptibly` in `ProcessQueue` and `RouteInfoManager`
Posted by GitBox <gi...@apache.org>.
dugenkui03 commented on PR #4113:
URL: https://github.com/apache/rocketmq/pull/4113#issuecomment-1088502286
@MatrixHB I create new branch from **master** instead of develop branch, is this right operation?
I plan resolving the conflicts when the ’conflict change‘ merged into master.
--
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