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

feat: Enhancing debug information for HttpDataSource transfers #4303

Conversation

zub4t
Copy link
Contributor

@zub4t zub4t commented Jun 25, 2024

What this PR changes/adds

Appends the reason for the error to the message that will be sent to the user when the transfer fails

Why it does that

To improve the user experience by returning some feedback when the StreamResult has no message associated with it

Linked Issue(s)

Closes #4280

@zub4t zub4t force-pushed the feat/enhancing_debug_information_for_httpDataSource_transfers branch from a4732ad to 793dbe2 Compare June 25, 2024 21:12
@zub4t zub4t requested a review from jimmarino June 25, 2024 21:13
@ndr-brt ndr-brt self-requested a review June 26, 2024 06:31
@ndr-brt ndr-brt added the enhancement New feature or request label Jun 26, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.20%. Comparing base (7f20ba5) to head (f23e621).
Report is 334 commits behind head on main.

Files Patch % Lines
...onnector/dataplane/spi/pipeline/StreamFailure.java 50.00% 0 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4303      +/-   ##
==========================================
+ Coverage   71.74%   75.20%   +3.45%     
==========================================
  Files         919     1052     +133     
  Lines       18457    21116    +2659     
  Branches     1037     1181     +144     
==========================================
+ Hits        13242    15880    +2638     
+ Misses       4756     4720      -36     
- Partials      459      516      +57     

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

@zub4t zub4t force-pushed the feat/enhancing_debug_information_for_httpDataSource_transfers branch from 793dbe2 to a97eb64 Compare June 26, 2024 09:04
@zub4t zub4t requested a review from ndr-brt June 26, 2024 09:06
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.

unit tests should stay as close as possible to the code, I wouldn't add them on PipelineServiceImpl nor on ParallelSink but on StreamFailure directly.

@zub4t
Copy link
Contributor Author

zub4t commented Jun 26, 2024

unit tests should stay as close as possible to the code, I wouldn't add them on PipelineServiceImpl nor on ParallelSink but on StreamFailure directly.

I didn't do this because there is no test class dedicated to the StreamFailure class, even for most of the classes within the :spi:data-plane:data-plane-spi module there are no specific test classes. So should I create a StreamFailureTest class in this module?

@zub4t zub4t requested a review from ndr-brt June 26, 2024 09:22
@ndr-brt
Copy link
Member

ndr-brt commented Jun 26, 2024

unit tests should stay as close as possible to the code, I wouldn't add them on PipelineServiceImpl nor on ParallelSink but on StreamFailure directly.

I didn't do this because there is no test class dedicated to the StreamFailure class, even for most of the classes within the :spi:data-plane:data-plane-spi module there are no specific test classes. So should I create a StreamFailureTest class in this module?

Yes, there wasn't because it was not necessary, but now it will become

@zub4t zub4t force-pushed the feat/enhancing_debug_information_for_httpDataSource_transfers branch 2 times, most recently from 7881fee to a0982ae Compare June 26, 2024 12:37
@ndr-brt
Copy link
Member

ndr-brt commented Jun 26, 2024

please note that other tests around the codebase are broken

@zub4t zub4t force-pushed the feat/enhancing_debug_information_for_httpDataSource_transfers branch from a0982ae to 1b9d5ba Compare June 26, 2024 13:38
@zub4t zub4t requested review from jimmarino and ndr-brt June 26, 2024 13:43
@zub4t zub4t requested a review from jimmarino June 26, 2024 14:04
@zub4t zub4t requested a review from ndr-brt June 26, 2024 16:10
@jimmarino jimmarino self-requested a review June 26, 2024 20:23
@jimmarino
Copy link
Contributor

@zub4t Tests are failing, please fix them.

@zub4t zub4t force-pushed the feat/enhancing_debug_information_for_httpDataSource_transfers branch 3 times, most recently from 40343b5 to a0362cf Compare June 26, 2024 20:59
@zub4t zub4t force-pushed the feat/enhancing_debug_information_for_httpDataSource_transfers branch from a0362cf to a022b3e Compare June 26, 2024 21:18
@zub4t zub4t force-pushed the feat/enhancing_debug_information_for_httpDataSource_transfers branch from 317309e to a022b3e Compare June 26, 2024 21:39
@zub4t zub4t marked this pull request as draft June 27, 2024 09:10
@zub4t zub4t marked this pull request as ready for review July 2, 2024 09:22
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.

LGTM

@ndr-brt ndr-brt merged commit 44786b5 into eclipse-edc:main Jul 2, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancing debug information for HttpDataSource transfers
4 participants