Skip to content
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

feat: support consume back #56

Merged
merged 1 commit into from
Dec 14, 2018
Merged

feat: support consume back #56

merged 1 commit into from
Dec 14, 2018

Conversation

denghongcai
Copy link
Collaborator

@denghongcai denghongcai commented Nov 21, 2018

#34 support RECONSUME_LATER status, send back to broker.

@codecov
Copy link

codecov bot commented Nov 21, 2018

Codecov Report

Merging #56 into master will increase coverage by 0.93%.
The diff coverage is 91.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   90.59%   91.52%   +0.93%     
==========================================
  Files          35       35              
  Lines        1723     1806      +83     
==========================================
+ Hits         1561     1653      +92     
+ Misses        162      153       -9
Impacted Files Coverage Δ
lib/mq_client.js 88.27% <ø> (+0.34%) ⬆️
lib/producer/mq_producer.js 95.2% <100%> (+4.04%) ⬆️
lib/message/message_const.js 100% <100%> (ø) ⬆️
lib/mix_all.js 100% <100%> (+11.11%) ⬆️
lib/message/message.js 100% <100%> (ø) ⬆️
lib/mq_client_api.js 76.74% <71.42%> (+2.7%) ⬆️
lib/utils/index.js 96.55% <80%> (-3.45%) ⬇️
lib/consumer/mq_push_consumer.js 93.6% <92.3%> (+0.77%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1258c2...beb756c. Read the comment docs.

@denghongcai denghongcai changed the title [WIP] feat: support consume back feat: support consume back Dec 10, 2018
@denghongcai denghongcai force-pushed the feat-support-cosume-back branch 3 times, most recently from dc0796c to 11039f6 Compare December 10, 2018 06:27
@denghongcai denghongcai mentioned this pull request Dec 10, 2018
@denghongcai denghongcai force-pushed the feat-support-cosume-back branch 2 times, most recently from 6a2e213 to 13536e3 Compare December 10, 2018 07:03
@gxcsoccer
Copy link
Contributor

这个我晚点仔细看下

@@ -78,6 +78,7 @@ class MQProducer extends ClientConfig {
*/
async init() {
// this._topicPublishInfoTable.set(this.createTopicKey, new TopicPublishInfo());
await MQProducer.getDefaultProducer(this.options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里为啥要 getDefaultProducer ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

初始化一下,CLIENT_INNER_PRODUCER 的 Producer 是默认就要有的

})();
}

async _consumeMessageLoop(topic, needFilter, tagsSet, handler, subExpression) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这段现在好复杂

@gxcsoccer
Copy link
Contributor

consume back 和并发消费,其实可以分开 pr 的,改动都比较大

if (!msg.tags || !needFilter || tagsSet.includes(msg.tags)) {
try {
if (msg.reconsumeTimes < this.options.maxReconsumeTimes) {
await handler(msg, mq, pq);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里还是一条一条的串行的吧?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改了,你再看看

@denghongcai denghongcai force-pushed the feat-support-cosume-back branch 2 times, most recently from 37515ec to ad31907 Compare December 12, 2018 07:28
this.subscriptions.set(retryTopic, {
subscriptionData: retrySubscriptionData,
});
}
Copy link
Contributor

@gxcsoccer gxcsoccer Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个可以在最开始就初始化好,不用每次都做

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最开始初始化,没有注册过 handler 的情况下会导致找不到 handler

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这一段好像和 handler 无关的吧

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

应该是在初始化的时候就监听统一的 retryTopic,retryTopic 的 handler 比较特殊,就是从 this.subscriptions 里面找原始 topic 的 handler 吐给他

this.logger.error('[MQPushConsumer] send reconsume message failed, fall to local retry, msgId: %s', msg.msgId);
// 重试消息发送失败,本地重试
msg.reconsumeTimes++;
await this._sleep(5000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里如果真的失败的,会导致这批消息重新消费

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里还不如一只让他定时重发,直到成功。

因为直接跑错的效果也是类似的,还会导致其他消息重复消费

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里好像还没改

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

把上面的循环判断去掉了,永远重试


if (this.messageModel === MessageModel.CLUSTERING) {
const retryTopic = MixAll.getRetryTopic(this.consumerGroup);
loops.push((async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这样会重复注册 retryTopic 的消费逻辑吧?

@denghongcai denghongcai force-pushed the feat-support-cosume-back branch 3 times, most recently from 9d98427 to a65d043 Compare December 12, 2018 10:21

async consumeSingleMsg(handler, msg, mq, pq) {
try {
if (msg.reconsumeTimes < this.options.maxReconsumeTimes) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果超过 maxResconsumeTimes 应该有个报错吧

Copy link
Collaborator Author

@denghongcai denghongcai Dec 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个判断直接去掉了

// 并发消费任务
const consumeTasks = [];
for (const msg of msgs) {
utils.resetRetryTopic(msg, this.consumerGroup);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个应该放到 retryTopic 的订阅逻辑里吧?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

恩,改了,忘记删了

: msg.storeHost;
const thatConsumerGroup = consumerGroup ? consumerGroup : this.consumerGroup;
try {
await this._mqClient.consumerSendMessageBack(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consumerSendMessageBack 和 send retry message 有什么区别?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consumerSendMessageBack 不需要发消息体,send retry message 需要发

@denghongcai denghongcai force-pushed the feat-support-cosume-back branch 3 times, most recently from c8b0c7f to a12360d Compare December 14, 2018 07:22
this.emit('error', err);
this.logger.error('[MQPushConsumer] send reconsume message failed, fallback to local retry, msgId: %s', msg.msgId);
// 重试消息发送失败,本地重试
msg.reconsumeTimes++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是不是应该挪到上个 catch 里?

@gxcsoccer
Copy link
Contributor

其他 +1

@gxcsoccer gxcsoccer merged commit 9acff9b into master Dec 14, 2018
@gxcsoccer gxcsoccer deleted the feat-support-cosume-back branch December 14, 2018 10:38
@gxcsoccer
Copy link
Contributor

3.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants