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

Make sinks, sources, and pipelines generic on their errors #66

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

passcod
Copy link
Contributor

@passcod passcod commented Nov 29, 2024

Implementation of #29.

  • PipelineError becomes generic: PipelineError<SrcErr: SourceError, SnkErr: SinkError>
  • We introduce the SourceError and SinkError marker traits which the respective errors must implement. This is a slight workaround because PipelineError<SrcErr: Error, SnkErr: Error> doesn't work (I tried, the type system doesn't like it), but it doesn't harm ergonomics too much.
  • We add associated Error types on the Sink, BatchSink, and Source traits, with bounds on the respective SinkError and SourceError traits.
  • We add a CommonSourceError type which encodes the common error variants implemented in the pipeline/source glue code, rather than the ones defined by the individual source/sink types.
  • For convenience we add two never types: InfallibleSourceError and InfallibleSinkError and implement the SourceError and SinkError traits on them. This saves making your own useless error types when implementing sinks and sources that don't error.
  • Finally we move the DuckDB and BigQuery errors to their own types, implement the traits, and wire everything up correctly.

Something remarkable with this (which even surprised me when I wrote it!) is that while this is a breaking API change, it doesn't require any change of the typical downstream code, as illustrated by this PR not changing anything in the replicator crate. Type inference takes care of it.

@passcod passcod marked this pull request as ready for review November 29, 2024 08:06
@passcod passcod force-pushed the associated-error-types branch from 4fadd46 to 388e84d Compare November 30, 2024 08:19
@imor
Copy link
Contributor

imor commented Dec 1, 2024

Hey @passcod thanks for the PR. I haven't been able to find time in the last few days but I think I'll have some time tomorrow to review.

Copy link
Contributor

@imor imor left a comment

Choose a reason for hiding this comment

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

The PR looks great, thanks for the effort @passcod. The only question I had was about explicit map_errs added, could these have been avoided? In any case I'm merging it now and will see if these map_errs can be removed later or not.

@imor imor merged commit 8be000d into supabase:main Dec 2, 2024
6 checks passed
@imor
Copy link
Contributor

imor commented Dec 2, 2024

Oh I see why the #[from] annotations were changed and map_err were used, since both of them are Errors they conflict. I think this is also somehow related to your second point about the type system not liking using traits directly in the bounds. No worries, everything's good.

@imor imor mentioned this pull request Dec 2, 2024
4 tasks
@passcod passcod deleted the associated-error-types branch December 2, 2024 19:56
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