-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
not throwing config error #36577
not throwing config error #36577
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
val rootThrowable = ConnectorExceptionUtil.getRootConfigError(Exception(throwable)) | ||
|
||
if (ConnectorExceptionUtil.isConfigError(rootThrowable)) { | ||
terminate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this exit 1
or exit 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -28,6 +29,12 @@ class AirbyteExceptionHandler : Thread.UncaughtExceptionHandler { | |||
// https://docs.google.com/document/d/1ctrj3Yh_GjtQ93aND-WH3ocqGxsmxyC3jfiarrF6NY0/edit# | |||
LOGGER.error(logMessage, throwable) | |||
|
|||
val rootThrowable = ConnectorExceptionUtil.getRootConfigError(Exception(throwable)) | |||
|
|||
if (ConnectorExceptionUtil.isConfigError(rootThrowable)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since destinations also use this handler. Is the expected behavior shared by them as well?
cc: @jbfbell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me, so essentially if it's a config error, don't throw two errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly - it will still exit 1, but we are not sending a system_error trace message anymore to platform if it's a config error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
Can you file a follow-up with the platform to determine whether config errors should be retried? I think its ok for now
/publish-java-cdk
|
/publish-java-cdk
|
https://github.com/airbytehq/oncall/issues/4710
When we throw an exception in the source connector, it will go to IntegrationRunner.kt, in which it will generate an error trace message, where error type is dependent on the type of the exception, and then rethrow it, which is then to be caught in
AirbyteExceptionHandler
. In the handler it will then re-generate a trace message withsystem_error
.Thus if a configException goes into a source connector, we currently will generate 1 config_error trace message and 1 system_error trace message.
The fix is to not throw
system_error
inAirbyteExceptionHandler
if the root cause of exception is config error.Tested on postgres, behavior change now is that we will start retrying those syncs, which is not ideal but it aligns with the description from https://docs.google.com/document/d/1ywZazxJ5DO41V1afaHY_0UfKuMHeiqRqn8wborwTSkY/edit I'm not sure why we were not retrying before?