This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @aspacca? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

I'd like some more unit tests. I'm specifically worried about the case where a partial event is written to the file, the reader reads that, then the rest of event is written to the file. The current code will give us one event, because it waits for the separator. I'd like verification that the proposed code does that as well.

hello @leehinman, totally agree with that!

I thought that the bug could be handled only in the aws s3 input part (see 26eb38f)

then I saw the test failing and realised that it's more complicated, I've moved the PR to draft

basically we have to find how to handle this and this

the problem is that in the second err check, when we have n = 0, err = io.EOF from the next iteration, we return from the loop, and we fail to collect the buffer here

so we receive an empty message in the input anyway

I'm thinking about changing the approach of the bugfix and do a check in the input if the content contains newline separator at all, if not, let's not attach the linereader, what do you think about it? it's safer but going to be more expensive in terms of performance. a trade off could be to fallback to a non linereader only if zero events were emitted

I'm thinking about changing the approach of the bugfix and do a check in the input if the content contains newline separator at all, if not, let's not attach the linereader, what do you think about it? it's safer but going to be more expensive in terms of performance. a trade off could be to fallback to a non linereader only if zero events were emitted

I think the root of the problem is the current linereader is expecting data to be appended, so EOF doesn't mean the last data you could ever receive, just last for now. But in the case of s3 input, we actually have all the data, so EOF means the last data you could ever receive.

Could we maybe just have a flag where the line reader will treat EOF like a line separator? That way it would work for data that has no line separators and ones where the first n-1 events have line separators but the nth one doesn't. I'm having a hard time coming up with a good heuristic for determining append vs non-append case, which is why I'm leaning towards a flag.

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b issue-30436 upstream/issue-30436
git merge upstream/main
git push upstream issue-30436

@leehinman please, look a the current solution

if you are not satisfied I will add the flag for EOF

my only concern in the last commit I've pushed is that I have to use a TeeReader, and that can be very expensive

hi @leehinman , did you have the chance to review my changes? thanks