Skip to content

[fix][io] Not restart instance when kafka source poll exception.#20795

Merged
shibd merged 2 commits intoapache:masterfrom
shibd:fix_kafka_source
Jul 13, 2023
Merged

[fix][io] Not restart instance when kafka source poll exception.#20795
shibd merged 2 commits intoapache:masterfrom
shibd:fix_kafka_source

Conversation

@shibd
Copy link
Copy Markdown
Member

@shibd shibd commented Jul 13, 2023

Motivation

When Kafka source poll message exception, it will throw it to the function framework, and not trigger the restart function instance.

After #20424, it just closes the Kafka source connector poll thread, and not will trigger restart instance.

Modifications

  • When Kafka consumer gets an exception, call notifyError to notify this exception, and when the function framework next calls consumer.read method, it will trigger instance restart.

Verifying this change

  • Change closeConnectorWhenUnexpectedExceptionThrownTest to cover it.

Documentation

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

Matching PR in forked repository

@shibd shibd self-assigned this Jul 13, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jul 13, 2023
@shibd shibd added release/2.10.5 release/2.11.2 release/3.0.2 type/bug The PR fixed a bug or issue reported a bug labels Jul 13, 2023
@Technoboy- Technoboy- added this to the 3.1.0 milestone Jul 13, 2023
Comment thread pulsar-io/kafka/src/main/java/org/apache/pulsar/io/kafka/KafkaAbstractSource.java Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 13, 2023

Codecov Report

❌ Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.12%. Comparing base (f69e7dd) to head (7d8f05c).
⚠️ Report is 1814 commits behind head on master.

Files with missing lines Patch % Lines
...rg/apache/pulsar/io/kafka/KafkaAbstractSource.java 55.55% 6 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20795       +/-   ##
=============================================
+ Coverage     36.82%   73.12%   +36.30%     
- Complexity    12123    32128    +20005     
=============================================
  Files          1689     1867      +178     
  Lines        129452   139047     +9595     
  Branches      14123    15299     +1176     
=============================================
+ Hits          47675   101685    +54010     
+ Misses        75465    29303    -46162     
- Partials       6312     8059     +1747     
Flag Coverage Δ
inttests 24.09% <0.00%> (-0.08%) ⬇️
systests 25.02% <0.00%> (+0.02%) ⬆️
unittests 72.41% <55.55%> (+40.38%) ⬆️

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

Files with missing lines Coverage Δ
...rg/apache/pulsar/io/kafka/KafkaAbstractSource.java 73.28% <55.55%> (+73.28%) ⬆️

... and 1431 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.

@shibd
Copy link
Copy Markdown
Member Author

shibd commented Jul 13, 2023

@Technoboy- @poorbarcode Thanks for the feedback.

About this comment: #20795 (comment)

I refactor the start logic:

  1. Let consumer.subscribe run on the start thread, which can quickly fail when subscribe exception.
  2. Fix InterruptedException will be caught: when encountering an InterruptedException should be thrown it.

PTAL.

@shibd shibd requested review from Technoboy- and poorbarcode July 13, 2023 08:22
Copy link
Copy Markdown
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants