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.

Sign up for GitHub

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

Jump to bottom

[feat] Support Dead Letter Topic. #139

Merged
merged 1 commit into from
Jan 19, 2023
Merged

[feat] Support Dead Letter Topic. #139

merged 1 commit into from
Jan 19, 2023

Conversation

shibd
Copy link
Member

@shibd shibd commented Dec 6, 2022 •
edited
Loading

Master Issue: #114

Motivation

#77, #114

Modifications

  • Add DeadLetterPolicy. When the message redeliveryCount > DeadLetterPolicy.maxRedeliveryCount, the message is sent to the DLQ topic and ack this msg.

Verifying this change

This change added tests and can be verified as follows:

  1. Add DeadLetterQueueTest to verify Send to DLQ, according to the following scenarios(Cartesian product of the following conditions):
  • Producer batch [enabled, disable]
  • Topic is [single, multi]
  • Consumer sub type [Shared, Key_Shard]
  • Trigger by [unack, ack_timeout]

  1. Add DeadLetterQueueTest.testInitSubscription to cover init DLQ topic subscription: [PIP-124] Create init subscription before sending message to DLQ  pulsar#13355

  2. Add DeadLetterQueueTest.testAutoSetDLQTopicName to cover default DLQ topic name.

  3. Add DeadLetterQueueTest.testDLQWithSchema to cover producer and consumer with the schema scenario.

  4. If a topic has a schema, and the consumer uses AUTO_CONSUME schema, and DLQTopic also has the same schema. In this case, the DLQ producer fails to create(IncompatibleSchema). Refer to java client same issue: Dead letter producer does not handle AUTO_CONSUME_SCHEMA pulsar#9935.
    Solution: Depending on [feat] Support auto download schema when create producer. #157, After this, DLQ producers use the autoDownloadSchema param when consumers use AUTO_CONSUM.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@shibd shibd force-pushed the dlq branch 2 times, most recently from 326d60d to 808d0dc Compare December 9, 2022 03:42
@shibd shibd mentioned this pull request Dec 12, 2022
Closed
4 tasks
@shibd shibd requested a review from BewareMyPower December 20, 2022 03:43
@shibd shibd mentioned this pull request Dec 26, 2022
Merged
4 tasks
@BewareMyPower
Copy link
Contributor

@Anonymitaet Could you help review the documents part (i.e. the API docs in include/pulsar/*.h?

@BewareMyPower BewareMyPower added this to the 3.2.0 milestone Jan 5, 2023
@BewareMyPower BewareMyPower added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 5, 2023
BewareMyPower
BewareMyPower requested changes Jan 5, 2023
View reviewed changes
lib/ConsumerImpl.cc Outdated Show resolved Hide resolved
BewareMyPower
BewareMyPower reviewed Jan 5, 2023
View reviewed changes
lib/ConsumerImpl.cc Outdated Show resolved Hide resolved
lib/ConsumerImpl.cc Outdated Show resolved Hide resolved
lib/ConsumerImpl.cc Outdated Show resolved Hide resolved
BewareMyPower
BewareMyPower reviewed Jan 5, 2023
View reviewed changes
tests/DeadLetterQueueTest.cc Outdated Show resolved Hide resolved
tests/DeadLetterQueueTest.cc Outdated Show resolved Hide resolved
tests/DeadLetterQueueTest.cc Outdated Show resolved Hide resolved
tests/DeadLetterQueueTest.cc Outdated Show resolved Hide resolved
tests/DeadLetterQueueTest.cc Outdated Show resolved Hide resolved
Anonymitaet
Anonymitaet reviewed Jan 6, 2023
View reviewed changes
include/pulsar/ConsumerConfiguration.h Outdated Show resolved Hide resolved
include/pulsar/ConsumerConfiguration.h Outdated Show resolved Hide resolved
include/pulsar/ConsumerConfiguration.h Outdated Show resolved Hide resolved
include/pulsar/ConsumerConfiguration.h Outdated Show resolved Hide resolved
include/pulsar/ConsumerConfiguration.h Outdated Show resolved Hide resolved
include/pulsar/ConsumerConfiguration.h Outdated Show resolved Hide resolved
include/pulsar/DeadLetterPolicyBuilder.h Outdated Show resolved Hide resolved
include/pulsar/DeadLetterPolicyBuilder.h Outdated Show resolved Hide resolved
include/pulsar/DeadLetterPolicyBuilder.h Outdated Show resolved Hide resolved
include/pulsar/DeadLetterPolicyBuilder.h Outdated Show resolved Hide resolved
@shibd
Copy link
Member Author

shibd commented Jan 6, 2023

@BewareMyPower @Anonymitaet Thanks for your reviews. All suggestions have been fixed. PTAL.

@Anonymitaet
Copy link
Member

Thanks! LGTM from the doc perspective.

BewareMyPower
BewareMyPower requested changes Jan 10, 2023
View reviewed changes
test-conf/standalone-ssl.conf Outdated Show resolved Hide resolved
@BewareMyPower
Copy link
Contributor

BTW, let's address comments in this PR in priority to #157.

@shibd shibd requested a review from BewareMyPower January 12, 2023 01:58
BewareMyPower
BewareMyPower reviewed Jan 17, 2023
View reviewed changes
lib/ProducerConfiguration.cc Outdated Show resolved Hide resolved
lib/ProducerConfigurationImpl.h Outdated Show resolved Hide resolved
BewareMyPower
BewareMyPower reviewed Jan 17, 2023
View reviewed changes
lib/Commands.h Outdated Show resolved Hide resolved
lib/ConsumerImpl.cc Outdated Show resolved Hide resolved
lib/ConsumerImpl.cc Show resolved Hide resolved
tests/DeadLetterQueueTest.cc Outdated Show resolved Hide resolved
tests/DeadLetterQueueTest.cc Outdated Show resolved Hide resolved
@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jan 17, 2023 •
edited
Loading

I have left the last comments. This PR overall LGTM. Please avoid committing such a huge patch next time. If a PR had many code changes, it would be very hard to review all of them. Some potential issues might not be exposed.

@shibd shibd self-assigned this Jan 18, 2023
@shibd
Copy link
Member Author

shibd commented Jan 19, 2023

I have left the last comments. This PR overall LGTM. Please avoid committing such a huge patch next time. If a PR had many code changes, it would be very hard to review all of them. Some potential issues might not be exposed.

Get it, Thanks for your professional reviews!

@shibd shibd requested a review from BewareMyPower January 19, 2023 03:17
@BewareMyPower BewareMyPower merged commit 9ed6a45 into apache:main Jan 19, 2023
BewareMyPower pushed a commit that referenced this pull request Jan 19, 2023
@shibd
[feat] Support auto download schema when create producer. (#157)
96507ce 
### Motivation
 If a schema exists in a topic, but the producer does not know it. He still wanted to send data to the topic. (The scenario in which the DLQ sends a message: Please refer #139 **[Verifying this change][5]**)


### Modifications
- When creating a producer with `autoDownloadSchema` param, try to get the schema of that topic and use it.
- Support `getSchema` on `LookupService`(HTTP and Binary).


### Verifying this change

  - Add `LookupServiceTest` unit test to cover `getSchema` logic.
  - Add `SchemaTest.testAutoDownloadSchema` unit test to cover creating producer success when the topic has a schema.

Co-authored-by: Baodi Shi <[email protected]>
@shibd shibd mentioned this pull request Feb 22, 2023
Open
This was referenced Mar 21, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anonymitaet Anonymitaet Anonymitaet left review comments

@BewareMyPower BewareMyPower BewareMyPower approved these changes

Assignees

@shibd shibd

Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@shibd @BewareMyPower @Anonymitaet