Repository navigation
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi benthos team,
This is a real issue I met in our production env, so I created this PR to handle it.
scanner/csv: don't skip read errors with
continue_on_errorProblem
With
continue_on_error: true, thecsvscanner skips every error that isn'tio.EOF, including errors coming from the underlying reader, not only CSV parse errors:A read error from the source is not a bad row that can be stepped over. Readers such as
an HTTP response body (
net/httpkeeps the first read error,bodyEOFSignal.rerr)return the same error on every later
Read().encoding/csvthen returns asingle empty field plus that error each time. The result:
(
{"<first header>": ""}) as fast as the pipeline can consume them;io.EOFis never reached, so the input never finishes the object, never acks it andnever moves on;
lives on the message's error flag.
The loop only stops when the process restarts.
How I hit it
An
aws_s3input in SQS mode reading a large|-delimited CSV withcontinue_on_error: true(Redpanda Connect 4.48 / benthos v4.44.1). The S3 downloadconnection broke partway through the object. The pod logged real rows, then switched to
nothing but empty rows for about 24 hours, at full CPU, until it was restarted. A restart
fixed it because the redelivered SQS message triggered a new
GetObjecton a newconnection.
Reproduced locally with an HTTP server that sends two rows and then resets the
connection, read through
http_clientwithstream.scanner.csv:continue_on_errortruefalseFailed to read message: read tcp ...: connection reset by peerFix
Skip an error only when it is a CSV parse error (
*csv.ParseError, e.g.ErrFieldCount,ErrQuote,ErrBareQuote), which is what the option is documented for. Any othererror is returned as normal, as if
continue_on_errorwerefalse:io.EOFis not a*csv.ParseError, so end-of-input behaviour is unchanged and theexplicit
io.EOFcheck is no longer needed.The
continue_on_errorfield description now says "fails to parse" instead of "fails toparse due to any error", and states that errors reading from the source are not skipped.
Behaviour change
reading continues.
continue_on_error: true: now returned fromNextBatch, so theinput handles them like any other read error. They get logged, and the input
reconnects, retries or nacks depending on the input. Before, they caused an infinite
loop that logged nothing.
continue_on_error: false: unchanged.Tests
Added
TestCSVScannerContinueOnErrorininternal/impl/pure/scanner_csv_test.go. Itreads a valid row, a wrong-field-count row and another valid row, then hits a reader that
returns
connection reset by peeron every read (iotest.ErrReader). It asserts that:csv.ErrFieldCount;NextBatch.On the old code the test fails with
Expected error with "connection reset by peer" in chain but got nil.go test ./internal/impl/pure/ -count=1