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

fix: data-plane-http - HttpSink loosing messages #3417 #3543

Conversation

drcgjung
Copy link
Contributor

What this PR changes/adds

This PR introduces a test case to verify the loss of messages by HttpSink under various partitioning conditions.
This PR removes an early return statement in the partition transfer loop of HttpSink which leads to the loss of messages.

Why it does that

Currently, HttpSink looses messages even in completely deterministic settings. It may only be safely applied with partition.size=1

Further notes

Linked Issue(s)

Closes #3417

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

…uce a multiplicity of parts with given binary content.
…ransfer of multiple message under various partition size settings. Demonstrates a bug in the current HttpSink partition transfer implementation.
…ooping over the messages of a partition in order not to loose messages.
@ndr-brt ndr-brt self-requested a review October 16, 2023 13:41
@ndr-brt ndr-brt added the bug Something isn't working label Oct 16, 2023
@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (12f1275) 72.24% compared to head (9e5a874) 72.45%.
Report is 8 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3543      +/-   ##
==========================================
+ Coverage   72.24%   72.45%   +0.21%     
==========================================
  Files         863      867       +4     
  Lines       17306    17388      +82     
  Branches      986      988       +2     
==========================================
+ Hits        12502    12598      +96     
+ Misses       4392     4375      -17     
- Partials      412      415       +3     
Files Coverage Δ
...onnector/dataplane/http/pipeline/HttpDataSink.java 88.00% <ø> (+8.83%) ⬆️

... and 21 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drcgjung drcgjung changed the title test(data-plane-spi)|fix(data-plane-http): HttpSink should not loose messages when transferring a partition #3417 fix: data-plane-http - HttpSink should not loose messages when transferring a partition #3417 Oct 16, 2023
@drcgjung drcgjung changed the title fix: data-plane-http - HttpSink should not loose messages when transferring a partition #3417 fix: data-plane-http - HttpSink loosing messages #3417 Oct 18, 2023
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

Is there some additional work to be done? If it's ready for review please mark it accordingly.

@drcgjung
Copy link
Contributor Author

Is there some additional work to be done? If it's ready for review please mark it accordingly.

I'm waiting for @git-masoud to check before I undraft it.

@drcgjung
Copy link
Contributor Author

I'm waiting for @git-masoud to check before I undraft it.

One point of discussion was the location of MultipleBinaryPartsDataSource.java

I placed it under the testFixtures of data-plane-spi because it is not specific to just data-plane-http and could be used to test all kinds of pipeline sinks ...

@drcgjung drcgjung marked this pull request as ready for review October 19, 2023 05:36
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

@ndr-brt ndr-brt merged commit 6a85970 into eclipse-edc:main Oct 19, 2023
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple partitions are not being transfered in Http Datapalne
3 participants