[fix][client] retry letter producer respect auto schema by tisonkun · Pull Request #19051 · apache/pulsar

tisonkun · 2022-12-25T02:42:17Z

This supersedes fix retryLetterProducer usr auto_chema #11550.
This closes Retry letter producer does not handle AUTO_CONSUME_SCHEMA #11474.

Verifying this change

Add a new test testAutoConsumeSchemaRetryLetter.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

codecov-commenter · 2022-12-25T03:13:06Z

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.53%. Comparing base (b42aed1) to head (1c507ed).
⚠️ Report is 3071 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/apache/pulsar/client/impl/ConsumerImpl.java	0.00%	10 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19051      +/-   ##
============================================
+ Coverage     47.11%   47.53%   +0.41%     
- Complexity    10595    10733     +138     
============================================
  Files           710      711       +1     
  Lines         69423    69459      +36     
  Branches       7449     7453       +4     
============================================
+ Hits          32709    33015     +306     
+ Misses        33037    32750     -287     
- Partials       3677     3694      +17

Flag	Coverage Δ
unittests	`47.53% <0.00%> (+0.41%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...va/org/apache/pulsar/client/impl/ConsumerImpl.java	`15.06% <0.00%> (-0.04%)`	⬇️

... and 61 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

congbobo184

could you please make the test simple and specific?

congbobo184 · 2022-12-25T09:27:29Z

+
+        @Cleanup
+        PulsarClient newPulsarClient = newPulsarClient(lookupUrl.toString(), 0);// Creates new client connection
+        Consumer<FooV2> deadLetterConsumer = newPulsarClient.newConsumer(Schema.AVRO(FooV2.class))


why we should use deadLetterConsumer?

I follow the test pattern from testRetryTopic and testAutoConsumeSchemaDeadLetter. Even testRetryTopic set up the DLQ.

May you directly submit a patch onto this pull request?

congbobo184 · 2022-12-25T09:29:18Z

+                .ackTimeout(1, TimeUnit.SECONDS)
+                .enableRetry(true)
+                .receiverQueueSize(100)
+                .deadLetterPolicy(DeadLetterPolicy.builder().maxRedeliverCount(maxRedeliveryCount).build())


we don't need to test dead letter right?

eolivelli

Lgtm

This approach is better.

The downside is that auto produce Schema validates the input, that is already validated and we are wadting some resources.

We can follow up with some enhancements

tisonkun · 2022-12-27T09:34:19Z

@congbobo184 I saw your patch and it seems code style check failed now. I'll try to fix it in hours.

tisonkun · 2022-12-27T09:34:27Z

Thank you!

poorbarcode · 2022-12-27T09:45:45Z

Hi @tisonkun

I found that the Files Changed have been dirty, could you deal with it?

tisonkun · 2022-12-27T10:02:26Z

@poorbarcode aha. It's due to a rebase on merge master commit. I can fix it later.

Signed-off-by: tison <wander4096@gmail.com> Co-authored-by: dibulidohu <dibulidohu@tencent.com>

Signed-off-by: tison <wander4096@gmail.com>

tisonkun · 2022-12-27T11:13:56Z

@poorbarcode updated.

FWIW, the process is:

Rename the local branch to tmp.
Checkout this pull request with gh pr checkout 19051
Reset to a ancient commit and merge master.
Cherry pick the effective commits from tmp (origin this pull request).
Force push.

tisonkun · 2022-12-27T13:23:24Z

A weird error: https://github.com/apache/pulsar/actions/runs/3787043645/jobs/6439462375

  Error:  Failed to execute goal org.jacoco:jacoco-maven-plugin:0.8.7:report (post-test) on project pulsar-broker: An error has occurred in JaCoCo report generation. Error while creating report: malformed input around byte 2 -> [Help 1]
  Error:  
  Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
  Error:  Re-run Maven using the -X switch to enable full debug logging.
  Error:  
  Error:  For more information about the errors and possible solutions, please read the following articles:
  Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
  Error: Process completed with exit code 1.

I'm trying rerun, but it seems an unexpected error. cc @yaalsn IIRC jacoco report is introduced by you.

tisonkun · 2022-12-27T13:53:28Z

Merging...

yaalsn · 2022-12-30T11:56:18Z

@tisonkun It seems that it passed after you rerun the job. Did you modify any code before the rerun?

tisonkun · 2022-12-30T12:06:24Z

@yaalsn No.

tisonkun added area/broker ready-to-test labels Dec 25, 2022

tisonkun requested review from codelipenghui, congbobo184, liudezhi2098 and lordcheng10 December 25, 2022 02:42

tisonkun self-assigned this Dec 25, 2022

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Dec 25, 2022

tisonkun mentioned this pull request Dec 25, 2022

fix retryLetterProducer usr auto_chema #11550

Closed

tisonkun added this to the 2.12.0 milestone Dec 25, 2022

tisonkun commented Dec 25, 2022

View reviewed changes

Comment thread pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

tisonkun added area/client and removed area/broker labels Dec 25, 2022

tisonkun changed the title ~~[fix][broker] retry letter producer respect auto schema~~ [fix][client] retry letter producer respect auto schema Dec 25, 2022

congbobo184 reviewed Dec 25, 2022

View reviewed changes

eolivelli approved these changes Dec 27, 2022

View reviewed changes

congbobo184 approved these changes Dec 27, 2022

View reviewed changes

tisonkun and others added 4 commits December 27, 2022 19:13

[fix][broker] retry letter producer respect auto schema

53d2a44

Signed-off-by: tison <wander4096@gmail.com> Co-authored-by: dibulidohu <dibulidohu@tencent.com>

spotbugs

b726060

Signed-off-by: tison <wander4096@gmail.com>

modify test

b85a6d0

fix: codestyle

1c507ed

Signed-off-by: tison <wander4096@gmail.com>

tisonkun force-pushed the fix-retry-letter-auto-schema branch from 3764ccb to 1c507ed Compare December 27, 2022 11:13

tisonkun merged commit 2877221 into apache:master Dec 27, 2022

tisonkun deleted the fix-retry-letter-auto-schema branch December 27, 2022 13:53

Uh oh!

Conversation

tisonkun commented Dec 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

Uh oh!

Uh oh!

codecov-commenter commented Dec 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

congbobo184 left a comment

Choose a reason for hiding this comment

Uh oh!

congbobo184 Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

tisonkun Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

congbobo184 Dec 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Dec 27, 2022

Uh oh!

tisonkun commented Dec 27, 2022

Uh oh!

poorbarcode commented Dec 27, 2022

Uh oh!

tisonkun commented Dec 27, 2022

Uh oh!

tisonkun commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tisonkun commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tisonkun commented Dec 27, 2022

Uh oh!

yaalsn commented Dec 30, 2022

Uh oh!

tisonkun commented Dec 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tisonkun commented Dec 25, 2022 •

edited

Loading

codecov-commenter commented Dec 25, 2022 •

edited

Loading

congbobo184 Dec 25, 2022 •

edited

Loading

tisonkun commented Dec 27, 2022 •

edited

Loading

tisonkun commented Dec 27, 2022 •

edited

Loading