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

More specific errors than Throwable for Consumer #1361

Draft
wants to merge 1 commit into
base: series/3.x
Choose a base branch
from

Conversation

svroonland
Copy link
Collaborator

@svroonland svroonland commented Nov 3, 2024

Mostly replaces Throwable with ConsumerError, a hierarchy of custom types we can later extend, and a fallback UnknownConsumerException. Some exceptions that can be thrown from the underlying KafkaConsumer which would indicate programming errors in zio-kafka are translated to fiber deaths (as per ZIO recommendations).

At some places we have more specific errors.

  • The partition stream can fail with just a PartitionStreamPullTimeout or DeserializationError.
  • Committing can fail with a CommitError, which is a CommitTimeout or UnknownCommitException.

I hope in the future these sub-hierarchies don't cause any conflicts.

Build still fails because test and bench projects have not been migrated yet, first want to gather some feedback on this change.

Implements #241

Implements #241

Build still fails because test and bench projects have not been migrated yet, first want to gather some feedback on this change.
@erikvanoosten
Copy link
Collaborator

I don't really care much for exception hierarchies unless there is something you can do for a specific error. And I guess that might indeed be the case here.

This is a non-binary compatible change which is okay, we'll simply increase the minor version number.

However, since ConsumerError does not extend Throwable, that makes this PR also a non-source compatible change. And that means we have to go to zio-kafka v3.0.0. I would prefer to define ConsumerError as a subtype of Throwable instead.

WDYT?

@svroonland
Copy link
Collaborator Author

Yes, my thought was that this should be released in v3. Doesn't have to be right now, we can shelve it for a while together with otther major changes we may want to do.

I would prefer not to make ConsumerError a subtype of Throwable, it feels like it goes against the idea of this PR.

@erikvanoosten
Copy link
Collaborator

Errors that are not a subtype of Throwable IMHO only makes sense when you always know how to handle them. That is definitely not the case here.

DX will go down the drain as you will need to sprinkle your code with .mapError(e => new RuntimeException(e)). And that is only after you realized that the compile errors are because of ZIO error channel type mismatches. Really, it sounds nice, but it only adds friction.

@svroonland
Copy link
Collaborator Author

Fair point. There should at least be a toException or something on ConsumerError. Let me think about it a bit more

@svroonland svroonland added this to the 3.0.0 milestone Nov 10, 2024
@svroonland svroonland changed the base branch from master to series/3.x November 10, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants