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 2020/05/22 21:41:47 UTC
[GitHub] [rocketmq] imaffe opened a new issue #2027: DefaultMessageStore#findConsumeQueue() shouldn't create new consumer queue when not found
imaffe opened a new issue #2027:
URL: https://github.com/apache/rocketmq/issues/2027
The issue tracker is **ONLY** used for bug report(feature request need to follow [RIP process](https://github.com/apache/rocketmq/wiki/RocketMQ-Improvement-Proposal)). Keep in mind, please check whether there is an existing same report before your raise a new one.
Alternately (especially if your communication is not a bug report), you can send mail to our [mailing lists](http://rocketmq.apache.org/about/contact/). We welcome any friendly suggestions, bug fixes, collaboration and other improvements.
Please ensure that your bug report is clear and that it is complete. Otherwise, we may be unable to understand it or to reproduce it, either of which would prevent us from fixing the bug. We strongly recommend the report(bug report or feature request) could include some hints as the following:
**BUG REPORT**
1. Please describe the issue you observed:
In DefaultMessageStore#findConsumeQueue(String topic, int queueId), I noticed it's implementation will create a new ConsumeQueue, if for given topic and queueId is not found in the map.
This implies the return value of findConsumeQueue() is never null.
However i searched all use cases of findConsumeQueue() like this one:
```
public long getMaxOffsetInQueue(String topic, int queueId) {
ConsumeQueue logic = this.findConsumeQueue(topic, queueId);
if (logic != null) {
long offset = logic.getMaxOffsetInQueue();
return offset;
}
return 0;
}
```
It checked whether the `logic` is null. I feel like from the use cases and the method name,findConsumeQueue() might be a read-only method. (it shouldn't change any internal state like creating, inserting queues into map). This method name might confuse developers when they simply want to do a query but mutated the states.
Only place that leveraged its write-access feature is here :
```
public void putMessagePositionInfo(DispatchRequest dispatchRequest) {
ConsumeQueue cq = this.findConsumeQueue(dispatchRequest.getTopic(), dispatchRequest.getQueueId());
cq.putMessagePositionInfoWrapper(dispatchRequest);
}
```
It didn't do any null check and are delegating findConsumeQueue() to create the consume queue if the topic-queueId pair was not found.
And this place relates to a bug #1852 .
It might be better if we either change the method name to findConsumeQueueAndCreateIfNotExist() or we return null when not found. But one problem is findConsumeQueueAndCreateIfNotExist() is referebced by 20+ lines so we need to be very careful to not break anything if we want to change the behaviour of this method.
- What did you do (The steps to reproduce)?
- What did you expect to see?
- What did you see instead?
2. Please tell us about your environment:
3. Other information (e.g. detailed explanation, logs, related issues, suggestions how to fix, etc):
**FEATURE REQUEST**
1. Please describe the feature you are requesting.
2. Provide any additional detail on your proposed use case for this feature.
2. Indicate the importance of this issue to you (blocker, must-have, should-have, nice-to-have). Are you currently using any workarounds to address this issue?
4. If there are some sub-tasks using -[] for each subtask and create a corresponding issue to map to the sub task:
- [sub-task1-issue-number](example_sub_issue1_link_here): sub-task1 description here,
- [sub-task2-issue-number](example_sub_issue2_link_here): sub-task2 description here,
- ...
----------------------------------------------------------------
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] lizhiboo commented on issue #2027: DefaultMessageStore#findConsumeQueue() shouldn't create new consumer queue when not found
Posted by GitBox <gi...@apache.org>.
lizhiboo commented on issue #2027:
URL: https://github.com/apache/rocketmq/issues/2027#issuecomment-634642620
> The issue tracker is **ONLY** used for bug report(feature request need to follow [RIP process](https://github.com/apache/rocketmq/wiki/RocketMQ-Improvement-Proposal)). Keep in mind, please check whether there is an existing same report before your raise a new one.
>
> Alternately (especially if your communication is not a bug report), you can send mail to our [mailing lists](http://rocketmq.apache.org/about/contact/). We welcome any friendly suggestions, bug fixes, collaboration and other improvements.
>
> Please ensure that your bug report is clear and that it is complete. Otherwise, we may be unable to understand it or to reproduce it, either of which would prevent us from fixing the bug. We strongly recommend the report(bug report or feature request) could include some hints as the following:
>
> Welcome any discussions~
> **BUG REPORT**
>
> 1. Please describe the issue you observed:
> In DefaultMessageStore#findConsumeQueue(String topic, int queueId), I noticed it's implementation will create a new ConsumeQueue, if for given topic and queueId is not found in the map.
>
> findConsumeQueue() implementation:
>
> ```
> public ConsumeQueue findConsumeQueue(String topic, int queueId) {
> ConcurrentMap<Integer, ConsumeQueue> map = consumeQueueTable.get(topic);
> if (null == map) {
> ConcurrentMap<Integer, ConsumeQueue> newMap = new ConcurrentHashMap<Integer, ConsumeQueue>(128);
> ConcurrentMap<Integer, ConsumeQueue> oldMap = consumeQueueTable.putIfAbsent(topic, newMap);
> if (oldMap != null) {
> map = oldMap;
> } else {
> map = newMap;
> }
> }
>
> ConsumeQueue logic = map.get(queueId);
> if (null == logic) {
> ConsumeQueue newLogic = new ConsumeQueue(
> topic,
> queueId,
> StorePathConfigHelper.getStorePathConsumeQueue(this.messageStoreConfig.getStorePathRootDir()),
> this.getMessageStoreConfig().getMappedFileSizeConsumeQueue(),
> this);
> ConsumeQueue oldLogic = map.putIfAbsent(queueId, newLogic);
> if (oldLogic != null) {
> logic = oldLogic;
> } else {
> logic = newLogic;
> }
> }
>
> return logic;
> }
> ```
>
> This implies the return value of findConsumeQueue() is never null.
>
> However i searched all use cases of findConsumeQueue() like this one:
>
> ```
> public long getMaxOffsetInQueue(String topic, int queueId) {
> ConsumeQueue logic = this.findConsumeQueue(topic, queueId);
> if (logic != null) {
> long offset = logic.getMaxOffsetInQueue();
> return offset;
> }
> return 0;
> }
> ```
>
> It checked whether the `logic` is null. I feel like from the use cases and the method name,findConsumeQueue() might be a read-only method. (it shouldn't change any internal state like creating, inserting queues into map). This method name might confuse developers when they simply want to do a query but mutated the states.
>
> Only place that leveraged its write-access feature is here :
>
> ```
> public void putMessagePositionInfo(DispatchRequest dispatchRequest) {
> ConsumeQueue cq = this.findConsumeQueue(dispatchRequest.getTopic(), dispatchRequest.getQueueId());
> cq.putMessagePositionInfoWrapper(dispatchRequest);
> }
> ```
>
> It didn't do any null check and are delegating findConsumeQueue() to create the consume queue if the topic-queueId pair was not found.
>
> And this place relates to a bug #1852 .
>
> It might be better if we either change the method name to findConsumeQueueAndCreateIfNotExist() or we return null when not found. But one problem is findConsumeQueueAndCreateIfNotExist() is referebced by 20+ lines so we need to be very careful to not break anything if we want to change the behaviour of this method.
>
> * What did you do (The steps to reproduce)?
> * What did you expect to see?
> * What did you see instead?
>
> 1. Please tell us about your environment:
> 2. Other information (e.g. detailed explanation, logs, related issues, suggestions how to fix, etc):
>
> **FEATURE REQUEST**
>
> 1. Please describe the feature you are requesting.
> 2. Provide any additional detail on your proposed use case for this feature.
> 3. Indicate the importance of this issue to you (blocker, must-have, should-have, nice-to-have). Are you currently using any workarounds to address this issue?
> 4. If there are some sub-tasks using -[] for each subtask and create a corresponding issue to map to the sub task:
>
> * [sub-task1-issue-number](example_sub_issue1_link_here): sub-task1 description here,
> * [sub-task2-issue-number](example_sub_issue2_link_here): sub-task2 description here,
> * ...
I support your suggestion, seperate findConsumeQueue() method to findConsumeQueueAndCreateIfNotExist() and findConsumeQueue(). Thus, it does not modify consumeQueueTable.
----------------------------------------------------------------
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