Skip to content

[improve][io] Add notifyError method on PushSource. #20791

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
Merged
shibd merged 1 commit into from
Jul 13, 2023
Merged

[improve][io] Add notifyError method on PushSource. #20791

shibd merged 1 commit into from
Jul 13, 2023

Conversation

@shibd
Copy link
Member

@shibd shibd commented Jul 12, 2023 •
edited
Loading

Motivation

If one source connector extends PushSource. An exception was encountered when this source connector consumed the source system data, it had no way to notify the function framework to restart this connector.

Modifications

  • Abstract BatchPushSource logic to AbstractPushSource.
  • Let PushSource to extends AbstractPushSource to extend a new method(notifyError).

Verifying this change

  • Add PushSourceTest to cover it.

Documentation

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

Matching PR in forked repository

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 12, 2023
@shibd shibd self-assigned this Jul 12, 2023
@shibd shibd changed the title Add notifyError method on PushSource. [improve][io] Add notifyError method on PushSource. Jul 12, 2023
@shibd shibd force-pushed the support_noti_error branch from 6315ef7 to a8af71f Compare July 12, 2023 14:20
@Technoboy- Technoboy- added this to the 3.1.0 milestone Jul 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2023

Codecov Report

Merging #20791 (a8af71f) into master (e96b339) will increase coverage by 39.65%.
The diff coverage is 80.64%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20791       +/-   ##
=============================================
+ Coverage     33.44%   73.09%   +39.65%     
- Complexity    12097    32115    +20018     
=============================================
  Files          1612     1867      +255     
  Lines        126568   139040    +12472     
  Branches      13824    15294     +1470     
=============================================
+ Hits          42328   101629    +59301     
+ Misses        78686    29346    -49340     
- Partials       5554     8065     +2511
Flag Coverage Δ
inttests 24.07% <0.00%> (-0.02%) ⬇️
systests 25.05% <3.22%> (?)
unittests 72.37% <80.64%> (+40.43%) ⬆️

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

Impacted Files Coverage Δ
...rg/apache/pulsar/broker/service/BrokerService.java 81.82% <ø> (+26.02%) ⬆️
.../org/apache/pulsar/io/core/AbstractPushSource.java 76.92% <76.92%> (ø)
...rg/apache/pulsar/client/impl/TopicListWatcher.java 66.66% <100.00%> (+43.47%) ⬆️
...ava/org/apache/pulsar/io/core/BatchPushSource.java 100.00% <100.00%> (+100.00%) ⬆️
...ain/java/org/apache/pulsar/io/core/PushSource.java 100.00% <100.00%> (+100.00%) ⬆️

... and 1515 files with indirect coverage changes

@shibd shibd merged commit ce28dda into apache:master Jul 13, 2023
@RobertIndie
Copy link
Member

RobertIndie commented Jul 13, 2023

@shibd Would this PR change the interface for the ecosystem component? If yes, I think we shouldn't add it to branch-3.0.
I remove the label release-3.0.2 first. But feel free to confirm it and add back the label if it's suitable to cherry-pick to the LTS branch.

@shibd shibd mentioned this pull request Jul 14, 2023
Merged
4 tasks
@shibd
Copy link
Member Author

shibd commented Jul 14, 2023

@shibd Would this PR change the interface for the ecosystem component? If yes, I think we shouldn't add it to branch-3.0. I remove the label release-3.0.2 first. But feel free to confirm it and add back the label if it's suitable to cherry-pick to the LTS branch.

Thanks for your reminder. I remove the release-x label, and patch a PIP design: #20807

This was referenced Jul 16, 2023
Merged
Merged
Merged
@nlu90
Copy link
Member

nlu90 commented Jul 18, 2023

We probably also need this in Sink connector to capture error happens when the actual write action happens

@shibd
Copy link
Member Author

shibd commented Jul 18, 2023

We probably also need this in Sink connector to capture error happens when the actual write action happens

Sure, I'll research a way to do it.

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

Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@RobertIndie RobertIndie Awaiting requested review from RobertIndie

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

Assignees

@shibd shibd

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@shibd @codecov-commenter @RobertIndie @nlu90 @Technoboy- @poorbarcode