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 2021/04/30 10:10:34 UTC
[GitHub] [rocketmq] andsel opened a new pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
andsel opened a new pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854
## What is the purpose of the change
This PR move common code used in two methods under the same path
## Brief changelog
- Introduce some exceptions for `org.apache.rocketmq.store.PutMessageStatus` to avoid the switch-return pattern moving to switch-throw
- move common code under the new method `appendPutMessageAndTrackStats`
## Verifying this change
`mvn verify`
Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] 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.
- [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.
- [ ] ~~Write necessary unit-test 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).~~
- [ ] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] codecov-commenter edited a comment on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-830004034
# [Codecov](https://codecov.io/gh/apache/rocketmq/pull/2854?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 [#2854](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2fea483) into [master](https://codecov.io/gh/apache/rocketmq/commit/b4240d5cea8d001c21b9c0d73f5aa700fcd0d568?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b4240d5) will **decrease** coverage by `0.86%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #2854 +/- ##
============================================
- Coverage 45.65% 44.79% -0.87%
+ Complexity 4382 3482 -900
============================================
Files 547 470 -77
Lines 37283 28868 -8415
Branches 5142 3740 -1402
============================================
- Hits 17022 12930 -4092
+ Misses 18149 14237 -3912
+ Partials 2112 1701 -411
```
| [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...e/rocketmq/client/impl/consumer/RebalanceImpl.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9SZWJhbGFuY2VJbXBsLmphdmE=) | `39.84% <0.00%> (-15.54%)` | :arrow_down: |
| [...apache/rocketmq/client/trace/TraceDataEncoder.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvdHJhY2UvVHJhY2VEYXRhRW5jb2Rlci5qYXZh) | `54.30% <0.00%> (-7.37%)` | :arrow_down: |
| [...a/org/apache/rocketmq/logging/inner/SysLogger.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9TeXNMb2dnZXIuamF2YQ==) | `28.57% <0.00%> (-5.72%)` | :arrow_down: |
| [...va/org/apache/rocketmq/client/trace/TraceBean.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvdHJhY2UvVHJhY2VCZWFuLmphdmE=) | `92.15% <0.00%> (-5.47%)` | :arrow_down: |
| [...java/org/apache/rocketmq/logging/inner/Logger.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9Mb2dnZXIuamF2YQ==) | `51.67% <0.00%> (-4.31%)` | :arrow_down: |
| [...nt/impl/consumer/ConsumeMessageOrderlyService.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9Db25zdW1lTWVzc2FnZU9yZGVybHlTZXJ2aWNlLmphdmE=) | `38.98% <0.00%> (-3.62%)` | :arrow_down: |
| [...cketmq/client/impl/consumer/RebalancePushImpl.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9SZWJhbGFuY2VQdXNoSW1wbC5qYXZh) | `35.51% <0.00%> (-3.27%)` | :arrow_down: |
| [...client/consumer/store/RemoteBrokerOffsetStore.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvY29uc3VtZXIvc3RvcmUvUmVtb3RlQnJva2VyT2Zmc2V0U3RvcmUuamF2YQ==) | `64.86% <0.00%> (-2.71%)` | :arrow_down: |
| [...ava/org/apache/rocketmq/filter/util/BitsArray.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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: |
| [...he/rocketmq/client/impl/consumer/ProcessQueue.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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==) | `59.53% <0.00%> (-2.33%)` | :arrow_down: |
| ... and [136 more](https://codecov.io/gh/apache/rocketmq/pull/2854/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/2854?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/2854?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 [b4240d5...2fea483](https://codecov.io/gh/apache/rocketmq/pull/2854?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] codecov-commenter edited a comment on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-830004034
# [Codecov](https://codecov.io/gh/apache/rocketmq/pull/2854?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 [#2854](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f9b3cbc) into [master](https://codecov.io/gh/apache/rocketmq/commit/b4240d5cea8d001c21b9c0d73f5aa700fcd0d568?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b4240d5) will **increase** coverage by `0.39%`.
> The diff coverage is `36.66%`.
[](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #2854 +/- ##
============================================
+ Coverage 45.65% 46.04% +0.39%
+ Complexity 4382 4324 -58
============================================
Files 547 548 +1
Lines 37283 36208 -1075
Branches 5142 4799 -343
============================================
- Hits 17022 16673 -349
+ Misses 18149 17453 -696
+ Partials 2112 2082 -30
```
| [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ain/java/org/apache/rocketmq/store/Exceptions.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0V4Y2VwdGlvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...main/java/org/apache/rocketmq/store/CommitLog.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0NvbW1pdExvZy5qYXZh) | `68.69% <59.45%> (+2.16%)` | `80.00 <4.00> (+1.00)` | |
| [...a/org/apache/rocketmq/logging/inner/SysLogger.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9TeXNMb2dnZXIuamF2YQ==) | `28.57% <0.00%> (-5.72%)` | `6.00% <0.00%> (-1.00%)` | |
| [...java/org/apache/rocketmq/logging/inner/Logger.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9Mb2dnZXIuamF2YQ==) | `51.19% <0.00%> (-4.79%)` | `28.00% <0.00%> (-1.00%)` | |
| [...n/java/org/apache/rocketmq/store/ha/HAService.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL2hhL0hBU2VydmljZS5qYXZh) | `68.54% <0.00%> (-4.66%)` | `18.00% <0.00%> (ø%)` | |
| [...ketmq/client/impl/consumer/PullMessageService.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9QdWxsTWVzc2FnZVNlcnZpY2UuamF2YQ==) | `71.11% <0.00%> (-4.45%)` | `8.00% <0.00%> (-1.00%)` | |
| [...lient/impl/consumer/DefaultMQPullConsumerImpl.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9EZWZhdWx0TVFQdWxsQ29uc3VtZXJJbXBsLmphdmE=) | `27.37% <0.00%> (-4.28%)` | `18.00% <0.00%> (-17.00%)` | |
| [...he/rocketmq/client/impl/consumer/ProcessQueue.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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==) | `58.60% <0.00%> (-3.26%)` | `31.00% <0.00%> (-1.00%)` | |
| [...rocketmq/client/impl/factory/MQClientInstance.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9mYWN0b3J5L01RQ2xpZW50SW5zdGFuY2UuamF2YQ==) | `49.70% <0.00%> (-3.14%)` | `90.00% <0.00%> (-4.00%)` | |
| [...ava/org/apache/rocketmq/filter/util/BitsArray.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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%)` | `30.00% <0.00%> (-1.00%)` | |
| ... and [22 more](https://codecov.io/gh/apache/rocketmq/pull/2854/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/2854?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/2854?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 [b4240d5...f9b3cbc](https://codecov.io/gh/apache/rocketmq/pull/2854?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] vongosling commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-844652215
@andsel you really reach the core codes in storage engine. Do you have a better way to verify that such refactorings are trustworthy, such as some unit tests, integration tests? I'm just thinking about optimizing this, It'll take some time. Please be patient. Also, you're welcome to participate :-)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] yuz10 commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
yuz10 commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r635764964
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -623,27 +646,22 @@ public long getBeginTimeInLock() {
mappedFile = this.mappedFileQueue.getLastMappedFile(0);
if (null == mappedFile) {
// XXX: warn and notify me
- log.error("create mapped file2 error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.CREATE_MAPEDFILE_FAILED, result));
+ log.error("create mapped file error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
+ throw new Exceptions.CreateMappedfileFailedException(result);
}
result = mappedFile.appendMessage(msg, this.appendMessageCallback);
break;
case MESSAGE_SIZE_EXCEEDED:
case PROPERTIES_SIZE_EXCEEDED:
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.MESSAGE_ILLEGAL, result));
+ throw new Exceptions.MessageIllegalException(result);
Review comment:
Is it possible that throw an Exception will affect performance?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] codecov-commenter commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-830004034
# [Codecov](https://codecov.io/gh/apache/rocketmq/pull/2854?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 [#2854](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6f1291f) into [master](https://codecov.io/gh/apache/rocketmq/commit/b4240d5cea8d001c21b9c0d73f5aa700fcd0d568?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b4240d5) will **increase** coverage by `0.38%`.
> The diff coverage is `36.66%`.
> :exclamation: Current head 6f1291f differs from pull request most recent head f9b3cbc. Consider uploading reports for the commit f9b3cbc to get more accurate results
[](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #2854 +/- ##
============================================
+ Coverage 45.65% 46.03% +0.38%
+ Complexity 4382 4328 -54
============================================
Files 547 548 +1
Lines 37283 36208 -1075
Branches 5142 4799 -343
============================================
- Hits 17022 16669 -353
+ Misses 18149 17459 -690
+ Partials 2112 2080 -32
```
| [Impacted Files](https://codecov.io/gh/apache/rocketmq/pull/2854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ain/java/org/apache/rocketmq/store/Exceptions.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0V4Y2VwdGlvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [...main/java/org/apache/rocketmq/store/CommitLog.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL0NvbW1pdExvZy5qYXZh) | `68.58% <59.45%> (+2.05%)` | `80.00 <4.00> (+1.00)` | |
| [...e/rocketmq/client/impl/consumer/RebalanceImpl.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9SZWJhbGFuY2VJbXBsLmphdmE=) | `45.81% <0.00%> (-9.57%)` | `32.00% <0.00%> (-4.00%)` | |
| [...org/apache/rocketmq/store/ha/WaitNotifyObject.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL2hhL1dhaXROb3RpZnlPYmplY3QuamF2YQ==) | `75.00% <0.00%> (-7.70%)` | `10.00% <0.00%> (-1.00%)` | |
| [...a/org/apache/rocketmq/logging/inner/SysLogger.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9TeXNMb2dnZXIuamF2YQ==) | `28.57% <0.00%> (-5.72%)` | `6.00% <0.00%> (-1.00%)` | |
| [...n/java/org/apache/rocketmq/store/ha/HAService.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-c3RvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3JvY2tldG1xL3N0b3JlL2hhL0hBU2VydmljZS5qYXZh) | `67.88% <0.00%> (-5.33%)` | `18.00% <0.00%> (ø%)` | |
| [...java/org/apache/rocketmq/logging/inner/Logger.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-bG9nZ2luZy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcm9ja2V0bXEvbG9nZ2luZy9pbm5lci9Mb2dnZXIuamF2YQ==) | `51.19% <0.00%> (-4.79%)` | `28.00% <0.00%> (-1.00%)` | |
| [...ketmq/client/impl/consumer/PullMessageService.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9QdWxsTWVzc2FnZVNlcnZpY2UuamF2YQ==) | `71.11% <0.00%> (-4.45%)` | `8.00% <0.00%> (-1.00%)` | |
| [...lient/impl/consumer/DefaultMQPullConsumerImpl.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9jb25zdW1lci9EZWZhdWx0TVFQdWxsQ29uc3VtZXJJbXBsLmphdmE=) | `27.37% <0.00%> (-4.28%)` | `18.00% <0.00%> (-17.00%)` | |
| [...rocketmq/client/impl/factory/MQClientInstance.java](https://codecov.io/gh/apache/rocketmq/pull/2854/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-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvaW1wbC9mYWN0b3J5L01RQ2xpZW50SW5zdGFuY2UuamF2YQ==) | `48.97% <0.00%> (-3.88%)` | `89.00% <0.00%> (-5.00%)` | |
| ... and [25 more](https://codecov.io/gh/apache/rocketmq/pull/2854/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/2854?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/2854?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 [b4240d5...f9b3cbc](https://codecov.io/gh/apache/rocketmq/pull/2854?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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] andsel commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r662960164
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -623,27 +646,22 @@ public long getBeginTimeInLock() {
mappedFile = this.mappedFileQueue.getLastMappedFile(0);
if (null == mappedFile) {
// XXX: warn and notify me
- log.error("create mapped file2 error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.CREATE_MAPEDFILE_FAILED, result));
+ log.error("create mapped file error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
+ throw new Exceptions.CreateMappedfileFailedException(result);
}
result = mappedFile.appendMessage(msg, this.appendMessageCallback);
break;
case MESSAGE_SIZE_EXCEEDED:
case PROPERTIES_SIZE_EXCEEDED:
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.MESSAGE_ILLEGAL, result));
+ throw new Exceptions.MessageIllegalException(result);
Review comment:
Hi, I moved from throwing exceptions to returning a couple with error or result
--
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] andsel commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r637376917
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -785,41 +786,24 @@ public long getBeginTimeInLock() {
}
public PutMessageResult putMessage(final MessageExtBrokerInner msg) {
- // Set the storage time
- msg.setStoreTimestamp(System.currentTimeMillis());
- // Set the message body BODY CRC (consider the most appropriate setting
- // on the client)
- msg.setBodyCRC(UtilAll.crc32(msg.getBody()));
+ setIPV6Flags(msg);
+
// Back to Results
AppendMessageResult result = null;
+ try {
+ result = appendPutMessageAndTrackStats(msg);
+ } catch (Exceptions.PutMessageException ex) {
+ return ex.toPutMessageResult();
+ }
+ PutMessageResult putMessageResult = new PutMessageResult(PutMessageStatus.PUT_OK, result);
- StoreStatsService storeStatsService = this.defaultMessageStore.getStoreStatsService();
-
- String topic = msg.getTopic();
- int queueId = msg.getQueueId();
-
- final int tranType = MessageSysFlag.getTransactionValue(msg.getSysFlag());
- if (tranType == MessageSysFlag.TRANSACTION_NOT_TYPE
- || tranType == MessageSysFlag.TRANSACTION_COMMIT_TYPE) {
- // Delay Delivery
- if (msg.getDelayTimeLevel() > 0) {
- if (msg.getDelayTimeLevel() > this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel()) {
- msg.setDelayTimeLevel(this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel());
- }
-
- topic = TopicValidator.RMQ_SYS_SCHEDULE_TOPIC;
- queueId = ScheduleMessageService.delayLevel2QueueId(msg.getDelayTimeLevel());
-
- // Backup real topic, queueId
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_TOPIC, msg.getTopic());
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_QUEUE_ID, String.valueOf(msg.getQueueId()));
- msg.setPropertiesString(MessageDecoder.messageProperties2String(msg.getProperties()));
+ handleDiskFlush(result, putMessageResult, msg);
+ handleHA(result, putMessageResult, msg);
- msg.setTopic(topic);
- msg.setQueueId(queueId);
- }
- }
+ return putMessageResult;
+ }
+ private void setIPV6Flags(MessageExtBrokerInner msg) {
Review comment:
DO you think is good to move the method `setIPV6Flags` into `MessageExt` class?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] andsel commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-914044059
Maybe @areyouok knows is #2889 contains all this changes?
--
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 #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-830003263
[](https://coveralls.io/builds/39257717)
Coverage increased (+0.02%) to 51.86% when pulling **f9b3cbc14470400ae56a842f1cad19892111358d on andsel:refactoring_remove_duplicated_code_commit_log** into **b4240d5cea8d001c21b9c0d73f5aa700fcd0d568 on apache: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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] andsel commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r637405322
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -623,27 +646,22 @@ public long getBeginTimeInLock() {
mappedFile = this.mappedFileQueue.getLastMappedFile(0);
if (null == mappedFile) {
// XXX: warn and notify me
- log.error("create mapped file2 error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.CREATE_MAPEDFILE_FAILED, result));
+ log.error("create mapped file error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
+ throw new Exceptions.CreateMappedfileFailedException(result);
}
result = mappedFile.appendMessage(msg, this.appendMessageCallback);
break;
case MESSAGE_SIZE_EXCEEDED:
case PROPERTIES_SIZE_EXCEEDED:
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.MESSAGE_ILLEGAL, result));
+ throw new Exceptions.MessageIllegalException(result);
Review comment:
from https://www.baeldung.com/java-exceptions-performance throwing exception is more costly than returning an error, but how many times do you expect the exception is raised?
The condition under which is raised are:
- message size too big
- the properties are too big
- mapped file doesn't exists.
Do you think they happen so often to generate a performance problem in throwing exception instead of returning error code?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] andsel commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-846378089
Hi @vongosling thank you a lot for your reply and sorry for my pushing, I've got the fear this was out of radar.
I haven't done any performance test, I think a JMH test for `asyncPutMessage` and `putMessage` could be help. I'll try to put it down something in this pr
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] coveralls edited a comment on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-830003263
[](https://coveralls.io/builds/41075619)
Coverage increased (+2.2%) to 54.055% when pulling **2fea483af72e3d7be4898f5fefe40b326e5abcac on andsel:refactoring_remove_duplicated_code_commit_log** into **b4240d5cea8d001c21b9c0d73f5aa700fcd0d568 on apache: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
[GitHub] [rocketmq] areyouok commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
areyouok commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-914281915
> Maybe @areyouok knows is #2889 contains all this changes?
I remove some duplicated code before my refactor in #2889. And it is include in the 4.9.1 version.
There is still many code are duplicated or unused.
--
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] yuz10 commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
yuz10 commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-913474741
@andsel maybe its included in #2889
--
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] yuz10 commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
yuz10 commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r664159239
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -785,41 +786,24 @@ public long getBeginTimeInLock() {
}
public PutMessageResult putMessage(final MessageExtBrokerInner msg) {
- // Set the storage time
- msg.setStoreTimestamp(System.currentTimeMillis());
- // Set the message body BODY CRC (consider the most appropriate setting
- // on the client)
- msg.setBodyCRC(UtilAll.crc32(msg.getBody()));
+ setIPV6Flags(msg);
+
// Back to Results
AppendMessageResult result = null;
+ try {
+ result = appendPutMessageAndTrackStats(msg);
+ } catch (Exceptions.PutMessageException ex) {
+ return ex.toPutMessageResult();
+ }
+ PutMessageResult putMessageResult = new PutMessageResult(PutMessageStatus.PUT_OK, result);
- StoreStatsService storeStatsService = this.defaultMessageStore.getStoreStatsService();
-
- String topic = msg.getTopic();
- int queueId = msg.getQueueId();
-
- final int tranType = MessageSysFlag.getTransactionValue(msg.getSysFlag());
- if (tranType == MessageSysFlag.TRANSACTION_NOT_TYPE
- || tranType == MessageSysFlag.TRANSACTION_COMMIT_TYPE) {
- // Delay Delivery
- if (msg.getDelayTimeLevel() > 0) {
- if (msg.getDelayTimeLevel() > this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel()) {
- msg.setDelayTimeLevel(this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel());
- }
-
- topic = TopicValidator.RMQ_SYS_SCHEDULE_TOPIC;
- queueId = ScheduleMessageService.delayLevel2QueueId(msg.getDelayTimeLevel());
-
- // Backup real topic, queueId
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_TOPIC, msg.getTopic());
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_QUEUE_ID, String.valueOf(msg.getQueueId()));
- msg.setPropertiesString(MessageDecoder.messageProperties2String(msg.getProperties()));
+ handleDiskFlush(result, putMessageResult, msg);
+ handleHA(result, putMessageResult, msg);
- msg.setTopic(topic);
- msg.setQueueId(queueId);
- }
- }
+ return putMessageResult;
+ }
+ private void setIPV6Flags(MessageExtBrokerInner msg) {
Review comment:
I dont think so, I just mean that another place could also use the function instead of writing the same code.
--
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 edited a comment on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-830003263
[](https://coveralls.io/builds/39258021)
Coverage increased (+0.07%) to 51.909% when pulling **f9b3cbc14470400ae56a842f1cad19892111358d on andsel:refactoring_remove_duplicated_code_commit_log** into **b4240d5cea8d001c21b9c0d73f5aa700fcd0d568 on apache: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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] andsel commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-913459959
HI @vongosling @yuz10 this PR was approved a couple of months ago, now it's conflicting, should resolve conflicts to get it merged or the changes this PR is proposing has already been merged with other PRs?
--
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] andsel commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r637376530
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -623,27 +646,22 @@ public long getBeginTimeInLock() {
mappedFile = this.mappedFileQueue.getLastMappedFile(0);
if (null == mappedFile) {
// XXX: warn and notify me
- log.error("create mapped file2 error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.CREATE_MAPEDFILE_FAILED, result));
+ log.error("create mapped file error, topic: " + msg.getTopic() + " clientAddr: " + msg.getBornHostString());
+ throw new Exceptions.CreateMappedfileFailedException(result);
}
result = mappedFile.appendMessage(msg, this.appendMessageCallback);
break;
case MESSAGE_SIZE_EXCEEDED:
case PROPERTIES_SIZE_EXCEEDED:
- beginTimeInLock = 0;
- return CompletableFuture.completedFuture(new PutMessageResult(PutMessageStatus.MESSAGE_ILLEGAL, result));
+ throw new Exceptions.MessageIllegalException(result);
Review comment:
It requires to create an extra object (the exception itself) and bring the stack trace of current thread, I've not any measure about performance loss. Do you think this is critical?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] yuz10 commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
yuz10 commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r635739093
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -785,41 +786,24 @@ public long getBeginTimeInLock() {
}
public PutMessageResult putMessage(final MessageExtBrokerInner msg) {
- // Set the storage time
- msg.setStoreTimestamp(System.currentTimeMillis());
- // Set the message body BODY CRC (consider the most appropriate setting
- // on the client)
- msg.setBodyCRC(UtilAll.crc32(msg.getBody()));
+ setIPV6Flags(msg);
+
// Back to Results
AppendMessageResult result = null;
+ try {
+ result = appendPutMessageAndTrackStats(msg);
+ } catch (Exceptions.PutMessageException ex) {
+ return ex.toPutMessageResult();
+ }
+ PutMessageResult putMessageResult = new PutMessageResult(PutMessageStatus.PUT_OK, result);
- StoreStatsService storeStatsService = this.defaultMessageStore.getStoreStatsService();
-
- String topic = msg.getTopic();
- int queueId = msg.getQueueId();
-
- final int tranType = MessageSysFlag.getTransactionValue(msg.getSysFlag());
- if (tranType == MessageSysFlag.TRANSACTION_NOT_TYPE
- || tranType == MessageSysFlag.TRANSACTION_COMMIT_TYPE) {
- // Delay Delivery
- if (msg.getDelayTimeLevel() > 0) {
- if (msg.getDelayTimeLevel() > this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel()) {
- msg.setDelayTimeLevel(this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel());
- }
-
- topic = TopicValidator.RMQ_SYS_SCHEDULE_TOPIC;
- queueId = ScheduleMessageService.delayLevel2QueueId(msg.getDelayTimeLevel());
-
- // Backup real topic, queueId
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_TOPIC, msg.getTopic());
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_QUEUE_ID, String.valueOf(msg.getQueueId()));
- msg.setPropertiesString(MessageDecoder.messageProperties2String(msg.getProperties()));
+ handleDiskFlush(result, putMessageResult, msg);
+ handleHA(result, putMessageResult, msg);
- msg.setTopic(topic);
- msg.setQueueId(queueId);
- }
- }
+ return putMessageResult;
+ }
+ private void setIPV6Flags(MessageExtBrokerInner msg) {
Review comment:
setIPV6Flags can also extract lines in putMessages, because MessageExtBatch and MessageExtBrokerInner both extends MessageExt
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] yuz10 commented on a change in pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
yuz10 commented on a change in pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#discussion_r664159239
##########
File path: store/src/main/java/org/apache/rocketmq/store/CommitLog.java
##########
@@ -785,41 +786,24 @@ public long getBeginTimeInLock() {
}
public PutMessageResult putMessage(final MessageExtBrokerInner msg) {
- // Set the storage time
- msg.setStoreTimestamp(System.currentTimeMillis());
- // Set the message body BODY CRC (consider the most appropriate setting
- // on the client)
- msg.setBodyCRC(UtilAll.crc32(msg.getBody()));
+ setIPV6Flags(msg);
+
// Back to Results
AppendMessageResult result = null;
+ try {
+ result = appendPutMessageAndTrackStats(msg);
+ } catch (Exceptions.PutMessageException ex) {
+ return ex.toPutMessageResult();
+ }
+ PutMessageResult putMessageResult = new PutMessageResult(PutMessageStatus.PUT_OK, result);
- StoreStatsService storeStatsService = this.defaultMessageStore.getStoreStatsService();
-
- String topic = msg.getTopic();
- int queueId = msg.getQueueId();
-
- final int tranType = MessageSysFlag.getTransactionValue(msg.getSysFlag());
- if (tranType == MessageSysFlag.TRANSACTION_NOT_TYPE
- || tranType == MessageSysFlag.TRANSACTION_COMMIT_TYPE) {
- // Delay Delivery
- if (msg.getDelayTimeLevel() > 0) {
- if (msg.getDelayTimeLevel() > this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel()) {
- msg.setDelayTimeLevel(this.defaultMessageStore.getScheduleMessageService().getMaxDelayLevel());
- }
-
- topic = TopicValidator.RMQ_SYS_SCHEDULE_TOPIC;
- queueId = ScheduleMessageService.delayLevel2QueueId(msg.getDelayTimeLevel());
-
- // Backup real topic, queueId
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_TOPIC, msg.getTopic());
- MessageAccessor.putProperty(msg, MessageConst.PROPERTY_REAL_QUEUE_ID, String.valueOf(msg.getQueueId()));
- msg.setPropertiesString(MessageDecoder.messageProperties2String(msg.getProperties()));
+ handleDiskFlush(result, putMessageResult, msg);
+ handleHA(result, putMessageResult, msg);
- msg.setTopic(topic);
- msg.setQueueId(queueId);
- }
- }
+ return putMessageResult;
+ }
+ private void setIPV6Flags(MessageExtBrokerInner msg) {
Review comment:
I dont think so, I just mean that another place could also use the function instead of writing the same code.
--
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] andsel commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-837955659
Hi @duhenglucky do you think this PR is missing something or is it good enough?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] andsel commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-842069932
@vongosling do you think this PR is good or should I fix something in it? Is it blocked by missing CLA (I think this is a small code change at the end)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [rocketmq] andsel commented on pull request #2854: [Issue #2853] Refactoring remove duplicated code in CommitLog putMessage/asyncPutMessage
Posted by GitBox <gi...@apache.org>.
andsel commented on pull request #2854:
URL: https://github.com/apache/rocketmq/pull/2854#issuecomment-842069932
@vongosling do you think this PR is good or should I fix something in it? Is it blocked by missing CLA (I think this is a small code change at the end)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org