fix(file source): fix message offset of opendal source #19721
Merged
+12
−1
Conversation
This file contains 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
stdrc
changed the title
xxchan
reviewed
Dec 9, 2024
| batch.push(SourceMessage { | ||
| key: None, | ||
| payload: Some(std::mem::take(&mut line_buf).into_bytes()), | ||
| offset: offset.to_string(), | ||
| offset: msg_offset, |
There was a problem hiding this comment.
from my PoV, the meaning of "offset" for file source is too hard to understand and evaluate. (And my brain refuse to think about it.) Is it possible to change to sth like (start,end)?
There was a problem hiding this comment.
Is it possible to change to sth like
(start,end)?
Agree, will consider doing this later.
xxchan
reviewed
Dec 9, 2024
Signed-off-by: Richard Chien <stdrc@outlook.com>
stdrc
force-pushed
the
rc/fix-file-source-offset
branch
from
f7b0682 to
099c8da
Compare
Will need over 1k files to reproduce the bug. I think the s3 test in main-cron workflow is enough. |
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.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
#19654 changed the
OpendalReader::stream_read_objectto yield lines instead of byte buffers, but there's a accidental change of the semantics of theSourceMessage::offsetfor file source, which caused main-cron to fail.The change is that, previously, when
OpendalReader::stream_read_objectyieldSourceMessages of byte buffers, theoffsetis the STARTIG position of the BYTES, and aftersplit_streamwhich splits these bytes by new-line, the newoffsetwill be set to the ENDING position of each LINE, which is also the STARTING position of the NEXT LINE. #19654 changed the reader, and the offset is changed to always representing the STARTING position of each LINE, hence causing the error.Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.