bulk-cdk: add ExceptionClassifier, revamp exception handling #44608
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
| operation = operationProvider.get()!! | ||
| } catch (e: Throwable) { | ||
| throw ConfigErrorException("Failed to initialize connector operation", e) | ||
| } |
There was a problem hiding this comment.
This change now causes Micronaut's instantiation of Operation to deferred to this try-catch block which handles any exceptions as ConfigErrorExceptions. This is a requirement to pass the CAT tests, there's a case in there that deliberately attempts a read with an invalid configured catalog and expects a TRACE ERROR message.
There was a problem hiding this comment.
Will this also mark system errors/other crashes as Config errors?
There was a problem hiding this comment.
Yes in theory. In practice, exceptions thrown at this point will be triggered by the CLI inputs exclusively.
| "Failed ${operation::class} operation execution." | ||
| } | ||
| } | ||
| outputConsumer.accept(exceptionClassifier.handle(e)) |
There was a problem hiding this comment.
The OutputConsumer no longer directly accepts exceptions, instead there's now an ExceptionClassifier which turns these exceptions into TRACE ERROR messages.
| .withStatus(AirbyteConnectionStatus.Status.FAILED), | ||
| ) | ||
| log.info { "Config check failed." } | ||
| return |
There was a problem hiding this comment.
All this SQLException logic had no business being in the general-purpose CheckOperation anyway. It's been moved to JdbcExceptionClassifier in the JDBC toolkit, where it belongs.
| import java.sql.SQLException | ||
|
|
||
| @Replaces(ExceptionClassifier::class) | ||
| class JdbcExceptionClassifier(@Value("\${${Operation.PROPERTY}}") operationName: String) : |
There was a problem hiding this comment.
Would extending this class in the connectors be the way to implement the error framework that @theyueli added e.g. PostgresSourceExceptionHandler?
There was a problem hiding this comment.
Yes and no. This class such as it is exists only to maintain existing functionality, which is pretty bare bones. I didn't want to add too much stuff to this PR. However, it's not really what we want; what we want is a bunch of classifiers among which @theyueli 's (the one with ConnectorErrorProfiles) would be one of them.
I see adding functionality as the logical next step. Micronaut really unlocks a lot of possibilities here, it should be trivial to add some kind of rule definitions into a yaml file such as https://github.com/airbytehq/airbyte-enterprise/blob/master/airbyte-integrations/connectors/source-firetruck/src/main/resources/application.yml
The obvious first choice is to have some kind of rules engine that works on SQLException vendor codes.
|
Thanks for taking a look! |
What
This change is required for CAT tests to pass for connectors built using the Bulk CDK.
This change also paves the way to declarative rules-based exception handling.
Informs https://github.com/airbytehq/airbyte-internal-issues/issues/9373
How
The change centers around the introduction of
ExceptionClassifierReview guide
Reading through the diff in the order that github presents it is actually pretty good.
User Impact
None
Can this PR be safely reverted and rolled back?