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

[Merged by Bors] - fix: properly handle produce operation failure #3405

Closed

Conversation

galibey
Copy link
Contributor

@galibey galibey commented Jul 20, 2023

The failure of produce operation was mistakenly treated as a failure of the transportation layer, which is not. That led to non-helpful error reporting to the user:

Producer error: failed to get record metadata

Fixed by not handling return error code too early.

@galibey galibey self-assigned this Jul 20, 2023
Copy link
Contributor

@digikata digikata left a comment

Choose a reason for hiding this comment

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

The change and description LGTM.

Just for my information, I tracked the error code thru the Poll::Ready. Either::Right logic, but I'm not sure I have a full handle on all the places on the path the error code is handled now? Is BatchMetadata::base_offset the only place? Just doing some extra reading because the (offset,error) pair has to be a little bit more explictly handled vs a Result type.

@galibey
Copy link
Contributor Author

galibey commented Jul 21, 2023

Is BatchMetadata::base_offset the only place?

To my understanding, yes. This method is called by wait function which you call when you want to know the result of your batch operation (Fluvio CLI does). Inside this BatchMetadata::base_offset if error_code is not None the batch metadata turns itself to failed state and Err is propagated up.

@galibey
Copy link
Contributor Author

galibey commented Jul 21, 2023

bors r+

bors bot pushed a commit that referenced this pull request Jul 21, 2023
The failure of produce operation was mistakenly treated as a failure of the transportation layer, which is not. That led to non-helpful error reporting to the user:
```bash
Producer error: failed to get record metadata
```
Fixed by not handling return error code too early.
@bors
Copy link

bors bot commented Jul 21, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title fix: properly handle produce operation failure [Merged by Bors] - fix: properly handle produce operation failure Jul 21, 2023
@bors bors bot closed this Jul 21, 2023
@galibey galibey deleted the feat/fix-producer-error-handling branch July 21, 2023 08:49
@digikata digikata added this to the 0.10.14 milestone Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants