Issue #1876 Implement line tracking for filesystem chunks in handleChunksWithError #4491
Conversation
|
|
didiercolens
force-pushed
the
fix/filesystem-line-number
branch
from
1a7728a to
e6a7e44
Compare
|
Thanks for taking a look at this - it's been a vexing issue for a while. But because it's been such a vexing issue, I'd like to ensure we do our due diligence with attempted fixes. Would you mind adding to the PR description:
Adding both of these things will allow reviewers to review the code in the context of your intention, and then, if we later merge it, provide a valuable historical record of the bug and the fix. Thanks! We really appreciate community contributions, especially for thorny problems like this one. I just want to ensure that the rest of us can keep up with you! |
There was a problem hiding this comment.
Just want a small change to the linesConsumed declaration and had a question about the proto.Clone call, but looks pretty good. I'll also second the asks from @rosecodym
| chunkSkel *sources.Chunk, | ||
| reporter sources.ChunkReporter, | ||
| ) error { | ||
| var linesConsumed int64 |
There was a problem hiding this comment.
Just linesConsumed := 0 is OK here; this will be an int which is 64-bit on all platforms we care about
| chunk := *chunkSkel | ||
| if chunk.SourceMetadata != nil { | ||
| if cloned, ok := proto.Clone(chunk.SourceMetadata).(*source_metadatapb.MetaData); ok { | ||
| chunk.SourceMetadata = cloned |
There was a problem hiding this comment.
I'm not sure what this is doing, but for various personal reasons I'm behind on sleep 😹 so it's likely I'm missing something obvious
| } | ||
| } | ||
|
|
||
| // updateFilesystemLineMetadata sets the 1-based starting line for filesystem chunks and |
5 participants
Description:
Attempt to address issue #1876. Note I used AI tools, and reviewed the code, but am not usually coding golang. Fix looks ok to me, but I can understand it does not meet the project standards.
Checklist:
make test-community)?make lintthis requires golangci-lint)? (reports issues but not on new code)