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

Support retry letter topic #359

Merged
merged 11 commits into from
Sep 9, 2020
Merged

Support retry letter topic #359

merged 11 commits into from
Sep 9, 2020

Conversation

wuYin
Copy link
Contributor

@wuYin wuYin commented Aug 24, 2020

Fixes #353

Motivation

Follow pulsar#6449 to support retry letter topic in go client

Modifications

  • Add retryRouter for sending reconsume messages to retry letter topic

  • Add ReconsumeLater(msg Message, delay time.Duration) to Consumer interface

  • Add configureable retry letter topic name in DLQPolicy

    type DLQPolicy struct {
    	// ...
    	// Name of the topic where the retry messages will be sent.
    	RetryLetterTopic string
    }

    enable it explicitly while creating consumer, default unenable

    type ConsumerOptions struct {
       // ...
       // Auto retry send messages to default filled DLQPolicy topics
       RetryEnable bool
    }
  • Add 2 TestRLQ* test cases

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

pulsar doc: retry-letter-topic

@wuYin wuYin changed the title Reconsume Support retry letter topic Aug 24, 2020
@wolfstudy wolfstudy requested review from merlimat and wolfstudy and removed request for merlimat August 25, 2020 01:14
@wolfstudy wolfstudy added this to the 0.3.0 milestone Aug 25, 2020
@wuYin wuYin marked this pull request as draft August 27, 2020 03:05
@wuYin wuYin marked this pull request as ready for review August 27, 2020 06:58
Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

Thanks @wuYin work for this, the change LGTM, just one comment, PTAL.

}
}
}()

Copy link
Member

Choose a reason for hiding this comment

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

@wuYin Why do we need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Background:

  • Retry topic producer, User topic consumer, they two use same connection instance to interact with broker.
  • Request channel, Response channel, they two are listened in a single select{} loop by a connection.

When message need to be redelivery:

  1. Retry topic producer called SendAsync(), wrapped User topic consumer AckID logic as callback into message handler.
  2. connection send cmd SEND to broker.
  3. Broker send cmd SEND_RECEIPT to connection.
  4. connection received and call callback
  5. callback called AckID(), then connection want to send cmd ACK to broker.
    in the meantime, select{} was stucked in step 3 and executing SEND_RECEIPT handler, incomingRequestsCh never be chosen, when it fulled with 10 Ack requests, finnally lead to dead block.

Asynchronized reuquest & response channels in connection, this block will be resolved.

deadblock

Copy link
Member

Choose a reason for hiding this comment

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

Cool, LGTM +1

@wolfstudy wolfstudy merged commit 3c523ba into apache:master Sep 9, 2020
@wuYin wuYin deleted the reconsume branch September 10, 2020 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Consumers Set Custom Retry Delay
2 participants