Bug: If one stream has an error, then all streams fail and no STATE messages are ever sent

[spacecowboy]

Singer SDK Version

0.11.1

Python Version

3.9

Bug scope

Targets (data type handling, batching, SQL object generation, etc.)

Operating System

Linux

Description

Scenario

• Your target is using incremental replication
• Your tap is sending multiple streams
• Each stream gets sent to its own Sink
• After a while, one of the Sinks throws an exception. For example if the tap sent a bad value in a record.
• This exception stops everything. The Target and all Sinks stop.

What is the actual result

Not a single STATE message is emitted by the target during this process even though it might have spent hours processing data already. Which means you need to start over from scratch. For ALL streams.

What is the expected result

The TARGET or SINKs should emit state messages periodically so state is not lost.

Code

No response

[spacecowboy]

I can mention that I'm using a BatchSink. It would be great if a state message was emitted after each batch.

[spacecowboy]

I propose changing the default _MAX_RECORD_AGE_IN_MINUTES in Target to something much smaller than 30 minutes.

[tayloramurphy]

@edgarrmondragon and @kgpayne thoughts here since you've been working on BATCH support and implementation

[edgarrmondragon]

@tayloramurphy this is independent from BATCH messages, there's a bit of term overloading (see #963 (comment) and
#1026).

@spacecowboy what's the target you're working with and are you able to override the value there? It might be a good idea to get heuristics or make the target batch size and wait limit configurable, similar to target-snowflake's batch_wait_limit_seconds and batch_size_rows.

[spacecowboy]

@spacecowboy what's the target you're working with and are you able to override the value there? It might be a good idea to get heuristics or make the target batch size and wait limit configurable, similar to target-snowflake's batch_wait_limit_seconds and batch_size_rows.

I have my own target-bigquery. I have overridden the value (in fact made it part of the config).

But in my opinion it just makes sense for the default value to be a lot smaller.

[aaronsteers]

@spacecowboy - re:

I propose changing the default _MAX_RECORD_AGE_IN_MINUTES in Target to something much smaller than 30 minutes.

Yes, this sounds like a good approach to me as well. 10 or 15 minutes is not often enough that we're spending a large percentage of time in the batch flush operations. Even 5 minutes might be okay here.

If we find that the max record age is not triggering as expected, that would be a separate bug we'd want to also address.

In your own target, what do you think of overriding _MAX_RECORD_AGE_IN_MINUTES to confirm the fix, and then we can merge back here as a PR if the STATE messages are emitted as expected?

Nm. I see above you've already confirmed. Yes, I'm in favor. 👍

[tayloramurphy]

@aaronsteers what's your view on priority here? Would love a weight estimate as well.

[aaronsteers]

@tayloramurphy and @spacecowboy - some scenarios affected here would be resolved by this which is merged and soon-to-be released:

• fix: Write a valid final state message at the end of each stream sync #1164

The relative priority for this would be low, since all is functioning as designed - or will be when the above releases. That said, I've added the Accepting Pull Requests label to invite contributions if anyone has time to pick this up.

Weight is probably 1 or 2 if we only reduce the trigger cadence defined in _MAX_RECORD_AGE_IN_MINUTES from 30 minutes to 15 or 10 - so we could also just decide to pick it up as low-hanging fruit.

[spacecowboy]

See #1192

[tayloramurphy]

Even better with the PR @spacecowboy 😄

@aaronsteers I've added the community contributed PR label and added it to the engineering board as well.