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

Abort on the commit log not being able to consume #124

Merged
merged 1 commit into from
Nov 14, 2023
Merged

Abort on the commit log not being able to consume #124

merged 1 commit into from
Nov 14, 2023

Conversation

huntc
Copy link
Collaborator

@huntc huntc commented Nov 13, 2023

We now abort if there are problems read from the commit log. All bets are off. If a problem occurs due to the data format having changed then it is on the programmer to upgrade the deserialisation.

Fixes #122

We now abort if there are problems read from the commit log. All bets are off. If a problem occurs due to the data format having changed then it is on the programmer to upgrade the deserialisation.
@huntc huntc added the enhancement New feature or request label Nov 13, 2023
@huntc huntc requested a review from patriknw November 13, 2023 22:15
@huntc huntc self-assigned this Nov 13, 2023
@@ -398,7 +397,7 @@ where
);
}
Err(e) => {
warn!("When consuming: {e:?}");
panic!("When consuming: {e:?}. Having to abort as this is unrecoverable.");
Copy link
Member

Choose a reason for hiding this comment

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

I think the error message should hint about what the user can do about it, such as implementing a serializer that can read or convert the old format.

Copy link
Collaborator Author

@huntc huntc Nov 14, 2023

Choose a reason for hiding this comment

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

I agree, and did think about that. However, the error may be due to a host of issues which are described by the string in the error (which will get printed here). For example, the secret store may not be available, or there’s a missing header value…

In summary, I’m not in a position to determine the actual cause. But I think we are ok given the description string of the error.

Copy link
Member

Choose a reason for hiding this comment

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

right, I think I didn't see that the cause is included

Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM

@huntc huntc merged commit c99ee5c into main Nov 14, 2023
1 check passed
@huntc huntc deleted the panic branch November 14, 2023 08:05
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.

Don't automatically skip events that cannot be deserialized
2 participants