-
Notifications
You must be signed in to change notification settings - Fork 11.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ROCKETMQ-111] fix possible MQClientException when query message before today #69
Conversation
Hi @yilingfeng , please modify your PR subject and mention your jira address in the description, you can follow this PR. #5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @yilingfeng , and please relate the JIRA issue.
Please @vongosling @lizhanhui help review.
Good catch. |
+1 |
From java doc, 0 of So this problem exists if we call the methods after 12:Am, the actualy hour will becomes 12 rather than 0, which may result in problems. Good catch and nice job. FYI: you could try with different system clock, which will give you a different result.
|
Good catch. I would add unit tests though ;) |
Thank you for your detailed and clear explanation. It was very helpful and understandable. @Jaskey |
@shroman , this PR will be merged soon, looking forward to your unit tests..😀😀😀😀 |
@zhouxinyu Probably you misunderstood what I meant :) -- this PR needs unit tests. I think it's the responsibility of the person who submitted PR to create them, whenever possible, since it reflects his/her intentions. |
Ok,I've added a unit test. @lizhanhui @shroman |
thanks @yilingfeng . Consider this PR has been merged, we will commit your unit-test in another commit. @shroman I have a same understand as @zhouxinyu about your unit-test said :-) |
@vongosling I meant "If I were you, I would ..." :) Anyway, I don't see why this pr can be merged without a unit test that can be easily written. I think we have to accept PRs with tests, except for the cases when they cannot be implemented. @yilingfeng I had a quick look, and here is what I think. |
Sorry, I’ve modified the unit tests. @shroman |
@yilingfeng Great 👍 But let's have a new PR for that, as @vongosling commented above (if I understood it correctly)? |
What does it mean? @vongosling @shroman |
Hi @yilingfeng @shroman , there is no need to open a new PR, the new commit will be merged to |
@zhouxinyu Thanks for clarifying it! ;) |
Changes Unknown when pulling e91b850 on yilingfeng:master into ** on apache:master**. |
2 similar comments
Changes Unknown when pulling e91b850 on yilingfeng:master into ** on apache:master**. |
Changes Unknown when pulling e91b850 on yilingfeng:master into ** on apache:master**. |
@yilingfeng Could you close this PR. We have merged this pull request 29 days ago. |
JIRA:https://issues.apache.org/jira/browse/ROCKETMQ-111
Using "cal.set(Calendar.HOUR,0);" when query message before today which may result in :
"org.apache.rocketmq.client.exception.MQClientException: CODE: 208 DESC: query message by key finished, but no message. "
"HOUR" is used for the 12-hour clock.This will cause the start time of the query message to be greater than the creation time of the message.
Implemenations:
Using "HOUR_OF_DAY" instead of "HOUR"."HOUR_OF_DAY" is used for the 24-hour clock.
Is that so?@Jaskey @zhouxinyu