Skip to content

Conversation

crepererum
Copy link
Collaborator

@crepererum crepererum commented Jul 27, 2022

On top of #159.

Closes #148.

@crepererum crepererum requested a review from tustvold July 27, 2022 13:04
@crepererum crepererum force-pushed the crepererum/issue148 branch from 9dd1c6b to a5a3fbf Compare August 1, 2022 09:22
@crepererum crepererum marked this pull request as ready for review August 1, 2022 09:22
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

It wasn't initially clear why the fields were separated, I've left some clarifications that I think might help?

payload: Option<ServerErrorPayload>,

/// Flags if the error was generated by the client to simulate some server behavior or workaround a bug.
is_virtual: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the intended use-case for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

debugging mostly

protocol_error: ProtocolError,

/// Server message provided by the broker, if any.
error_message: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this could be merged into ServerErrorResponse?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think most newer Kafka messages now have an error code and a human-readable string (i.e. this message). The response is mostly what else we could extract. So yeah, we could merge it but I guess the resulting enum will rather unhelpful because in many cases we don't have anything else than the error message.

@crepererum crepererum requested a review from tustvold August 2, 2022 11:58
@crepererum
Copy link
Collaborator Author

@tustvold PTAL

@crepererum crepererum added the automerge Instruct kodiak to merge the PR label Aug 2, 2022
@kodiakhq kodiakhq bot merged commit d56df16 into main Aug 2, 2022
@kodiakhq kodiakhq bot deleted the crepererum/issue148 branch August 2, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Instruct kodiak to merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More helpful OffsetOutOfRange error
2 participants