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/07/10 02:28:50 UTC
[GitHub] [rocketmq] StyleTang opened a new pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
StyleTang opened a new pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137
**Make sure set the target branch to `develop`**
## What is the purpose of the change
TraceDataEncoder add SubAfter trace bean timestamp
Mentioned in [PR-744](https://github.com/apache/rocketmq-externals/pull/744#discussion_r667088448) , the timeStamp and consumeGroup have been deleted by this commit.
(It is fine for deleting SubAfter consumeGroup because we can find consume group from SubBefore traceContext by messageId and requestId)
I use subBefore timestamp+costTime instead of subAfter timestamp, but it is not reasonable.
## Brief changelog
TraceDataEncoder add SubAfter trace bean timestamp
## Verifying this change
Unit Tests succeed
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] coveralls edited a comment on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-877597902
[](https://coveralls.io/builds/41264727)
Coverage increased (+0.006%) to 54.019% when pulling **09337745aefc5d207531e1e1b1de8ebaaa854d2a on StyleTang:trace-ui** into **589e87c278147ee17cbcaac334d8558a8ef98ea2 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] vongosling commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-882223403
Go ahead~ Designed well is what we have advocated. The previous disturbing is the obvious lack of this point.
--
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] StyleTang commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
StyleTang commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-882542027
@yuz10
Already add them back.
@vongosling
You are right. There is a small probability before and after trace not atomic exist.
If consumeMessageBefore has already been executed and the trace message has been sent by localDispatcher, then the system crashed, subAfter trace may be lost.
org.apache.rocketmq.client.impl.consumer.ConsumeMessageConcurrentlyService.ConsumeRequest#run
```java
if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) {
consumeMessageContext = new ConsumeMessageContext();
ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookBefore(consumeMessageContext);
}
try {
status = listener.consumeMessage(Collections.unmodifiableList(msgs), context);
} catch (Throwable e) {
}
// System crash happen
if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) { ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookAfter(consumeMessageContext);
}
```
--
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 #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-877597902
[](https://coveralls.io/builds/41470859)
Coverage increased (+0.007%) to 54.021% when pulling **e8a015233fb1c7168a7371b637d617e565294d36 on StyleTang:trace-ui** into **589e87c278147ee17cbcaac334d8558a8ef98ea2 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] coveralls edited a comment on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-877597902
[](https://coveralls.io/builds/41470859)
Coverage increased (+0.007%) to 54.021% when pulling **e8a015233fb1c7168a7371b637d617e565294d36 on StyleTang:trace-ui** into **589e87c278147ee17cbcaac334d8558a8ef98ea2 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] vongosling commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-878820580
Very responsible for this issue closed, 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] coveralls commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-877597902
[](https://coveralls.io/builds/41264636)
Coverage increased (+0.1%) to 54.132% when pulling **8bad001a0d26fa298b8ef08c76f3e1ec1fa38f80 on StyleTang:trace-ui** into **589e87c278147ee17cbcaac334d8558a8ef98ea2 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] coveralls edited a comment on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-877597902
[](https://coveralls.io/builds/41470859)
Coverage increased (+0.007%) to 54.021% when pulling **e8a015233fb1c7168a7371b637d617e565294d36 on StyleTang:trace-ui** into **589e87c278147ee17cbcaac334d8558a8ef98ea2 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] vongosling edited a comment on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
vongosling edited a comment on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-890763684
Yes, I would like to hear any compatible issues that resolved from the opinion of the other @duhengforever @zongtanghu I think this pr https://github.com/apache/rocketmq-externals/pull/769 would resolve the problem from the dashboard viewpoint.
--
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 #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
yuz10 commented on a change in pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#discussion_r668476980
##########
File path: client/src/main/java/org/apache/rocketmq/client/trace/TraceDataEncoder.java
##########
@@ -104,9 +104,8 @@
subAfterContext.setContextCode(Integer.parseInt(line[6]));
}
// compatible with the old version
- if (line.length >= 9) {
+ if (line.length >= 8) {
subAfterContext.setTimeStamp(Long.parseLong(line[7]));
- subAfterContext.setGroupName(line[8]);
Review comment:
The code here is used for compatible reason, if the message is produced by old versions of producer, there will be group name.
--
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] StyleTang commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
StyleTang commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-881803349
@yuz10 @duhenglucky Thanks for your comment.
For subAfterContext, in the previous version, it was already defined line[7] is TimeStamp and line[8] is GroupName, it means for compatible reason in new revision we should still follow this protocol.
The subAfter GroupName is duplicated with subBeofre, so based on performance considerations(I guess), it was removed it by https://github.com/apache/rocketmq/pull/3005/files
We have several ways to make it compatible.
### Option1 ( Prefer )
Just follow the previous version subAfter protocol.
( line[7] is TimeStamp and line[8] is GroupName )
##### Props
* The protocol remains consistent, easy to maintain
##### Cons
* In the future revision should add line[8] group name back, even if it is useless
### Option2
If line[8] GroupName was never used, just remove it as if it not exist.
##### Props
* Performance friendly (can remove the duplicated groupName)
##### Cons
* The protocol is changed (Not a good way)
* Compatible logic is complicated, old version line[8] is group name while new revision may be not. (The new revision content length should not be 9, if it is 9, we can’t tell what the value it is)
Do you have any suggestions for this?
--
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 #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
yuz10 commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-882151063
@StyleTang I prefer just add 2 back and keep the same with 4.9.0.
--
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] StyleTang commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
StyleTang commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-888291631
I think it is necessary for us to add Timestamp and GroupName back.
The reason is that send SubBefore and SubAfter message trace is not a atomic operation, we may only get SubBefore or SubAfter trace, If SubAfter trace don't have GroupName we don't know which subscription group consumed 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] vongosling edited a comment on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
vongosling edited a comment on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-883856062
@duhengforever @duhenglucky pls help to look at this issue, especially compatibility backword:-0
--
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 merged pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
yuz10 merged pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137
--
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] StyleTang commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
StyleTang commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-882542027
@yuz10
Already add them back.
@vongosling
You are right. There is a small probability before and after trace not atomic exist.
If consumeMessageBefore has already been executed and the trace message has been sent by localDispatcher, then the system crashed, subAfter trace may be lost.
org.apache.rocketmq.client.impl.consumer.ConsumeMessageConcurrentlyService.ConsumeRequest#run
```java
if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) {
consumeMessageContext = new ConsumeMessageContext();
ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookBefore(consumeMessageContext);
}
try {
status = listener.consumeMessage(Collections.unmodifiableList(msgs), context);
} catch (Throwable e) {
}
// System crash happen
if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) { ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookAfter(consumeMessageContext);
}
```
--
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] vongosling commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-883856062
@duhengforever Pls help to look at this issue, especially compatibility backword:-0
--
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] vongosling commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-890763684
Yes, I would like to hear any compatible issues that resolved from the opinion of the other @duhengforever @zongtanghu
--
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] vongosling commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
vongosling commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-882226328
For this problem, Who said before and after must be atomic exit?
--
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] StyleTang commented on pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
StyleTang commented on pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#issuecomment-882542027
@yuz10
Already add them back.
@vongosling
You are right. There is a small probability before and after trace not atomic exist.
If consumeMessageBefore has already been executed and the trace message has been sent by localDispatcher, then the system crashed, subAfter trace may be lost.
org.apache.rocketmq.client.impl.consumer.ConsumeMessageConcurrentlyService.ConsumeRequest#run
```java
if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) {
consumeMessageContext = new ConsumeMessageContext();
ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookBefore(consumeMessageContext);
}
try {
status = listener.consumeMessage(Collections.unmodifiableList(msgs), context);
} catch (Throwable e) {
}
// System crash happen
if (ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.hasHook()) { ConsumeMessageConcurrentlyService.this.defaultMQPushConsumerImpl.executeHookAfter(consumeMessageContext);
}
```
--
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] StyleTang commented on a change in pull request #3137: [ISSUE #3136] TraceDataEncoder add SubAfter trace bean timestamp
Posted by GitBox <gi...@apache.org>.
StyleTang commented on a change in pull request #3137:
URL: https://github.com/apache/rocketmq/pull/3137#discussion_r671595465
##########
File path: client/src/main/java/org/apache/rocketmq/client/trace/TraceDataEncoder.java
##########
@@ -104,9 +104,8 @@
subAfterContext.setContextCode(Integer.parseInt(line[6]));
}
// compatible with the old version
- if (line.length >= 9) {
+ if (line.length >= 8) {
subAfterContext.setTimeStamp(Long.parseLong(line[7]));
- subAfterContext.setGroupName(line[8]);
Review comment:
Get it.
I removed it because I thought this value is useless, maybe add the branch line.length >= 9 back is a better way.
```
// compatible with the old version
if (line.length >= 8) {
subAfterContext.setTimeStamp(Long.parseLong(line[7]));
}
if (line.length >= 9) {
subAfterContext.setGroupName(Long.parseLong(line[8]));
}
```
--
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