airbyte-common-workers: Collect trace message on failed connection_status

[alafanechere]

What

Closes #18865

Synchronous commands like SPEC, DISCOVER and CHECK can output trace messages that are not currently captured as failure reasons when the process exits with a 0 status code.

This PR aims at normalizing the behavior of these three commands:

• Do not consider the process exit code to characterize an error
• When a trace message is received: always add it to the job output
• When no trace message or catalog/connection status/spec is received -> throw a WorkerException

How

Implement an explicit logic in:

airbyte/airbyte-commons-worker/src/main/java/io/airbyte/workers/general/DefaultGetSpecWorker.java

Line 54 in 1f6c310

public ConnectorJobOutput run(final JobGetSpecConfig config, final Path jobRoot) throws WorkerException {

airbyte/airbyte-commons-worker/src/main/java/io/airbyte/workers/general/DefaultCheckConnectionWorker.java

Line 59 in 1f6c310

public ConnectorJobOutput run(final StandardCheckConnectionInput input, final Path jobRoot) throws WorkerException {

airbyte/airbyte-commons-worker/src/main/java/io/airbyte/workers/general/DefaultDiscoverCatalogWorker.java

Line 66 in 1f6c310

public ConnectorJobOutput run(final StandardDiscoverCatalogInput discoverSchemaInput, final Path jobRoot) throws WorkerException {

• Update the unit tests

[pedroslopez]

We talked about this on a call, but leaving a comment for posterity:

Looks good overall, but we should:

• Add testing for the changes on both WorkerUtils and DefaultCheckConnectionWorker
• Look into following the same approach of always looking for trace messages in the other synchronous commands (spec, discover)

[alafanechere]

@pedroslopez I tried to increase the shared logic between SPEC, DISCOVER and CHECK. I'd be interested in getting your opinion about the throwOnNoFailureReason approach.

[pedroslopez]

Looks good logically, most of my comments are around making things a bit clearer but non-blocking.

The only thing I'd like to see before approving is some additional tests for WorkerUtils / Default***Workers.

[alafanechere]

@pedroslopez I tried to make things a bit more readable and explicit:
We do not check the exit code anymore: we throw a WorkerException if:

• we can't find the object we're trying to read from the output (connection status, spec, catalog)
• AND we don't have a failure reason

This logic is kept within the worker classes (Default*Worker.java), the Worker utils class only has utils to extract useful objects from messages.

I'll add the tests if the overall logic looks good to you.

[pedroslopez]

Nice! I think this makes it much clearer. Only one comment about reassigning the job output that is non-blocking - just waiting on the tests 😄

[erohmensing]

I think this looks pretty good -- I added a few clarifying questions mostly for my own understanding! Following the logic, it looks like this

• looks for a connection status message
• looks for failure reasons in check connection messages
• sets the failure reason of the job if there's failure reasons
• if we got a connection status message, set the checkConnection output of the jobOutput
• if no connection status message and no failure reason, throw
• return the jobOutput

I followed the logic for checkConnection, and glanced over the other 2 jobs.

A few things that might have helped understand this better at first glance of a new code base:

• the existence CONNECTION_STATUS messages are being used to determine success, I think. could one consider a connectionStatus to be something like a connectionSuccess or connectionConfirmation?
• I wonder if it would be worth adding a method isSuccess or isSuccessful that returns output.getCheckConnection != null? (and similar for the other methods)? On spec, it's relatively intuitive that if getSpec is null, the job failed, but for getCheckConnection, as someone new to the code base, it feels like a method named getCheckConnection could contain information about a failed job or a successful job - I have to look at the code to see that it only gets set in the case of success.