feat: allow kinesis source start with timestamp #12241
Conversation
| } | ||
| KinesisOffset::Latest => (None, ShardIteratorType::Latest), | ||
| KinesisOffset::Earliest => (None, None, ShardIteratorType::TrimHorizon), | ||
| KinesisOffset::SequenceNumber(seq) => ( |
There was a problem hiding this comment.
I feel it would be better if we could give an warning to users, something like
"The sequence is only unique within a certain shard, so the semantics they expect may deviate from the reality, etc."
We give a warning instead of completely disabling it because we still want to keep backward compatibility
There was a problem hiding this comment.
I think it's a completely wrong design. Recommend removing it. (Yeah I know it's breaking change...)
Each data record has a sequence number that is unique per partition-key within its shard
There was a problem hiding this comment.
the warning here cannot propagate to the frontend. Let's make some breaking changes here.
There was a problem hiding this comment.
the warning here cannot propagate to the frontend. Let's make some breaking changes here.
If possible, I suggest we can leave the option here to avoid panic (perhaps print a warning log), but ignore the option, removing all related implementation code.
There was a problem hiding this comment.
I think there is no sequence_number option, unless the shards are fixed and pass sequence_numbers for each shard to rw. Otherwise, this feature doesn't make sense.
|
For the doc, we remove the existing and provide the new timestamp option |
Codecov Report
@@ Coverage Diff @@
## main #12241 +/- ##
==========================================
- Coverage 69.67% 69.67% -0.01%
==========================================
Files 1410 1410
Lines 236128 236137 +9
==========================================
- Hits 164526 164523 -3
- Misses 71602 71614 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
- Modify `start_position` variable to use `KinesisOffset::Timestamp` - Remove `seq_offset` field from `KinesisProperties` - Add `timestamp_offset` field with value `123456789098765432` to `KinesisProperties` - Remove `test_reject_redundant_seq_props` test function - Remove `seq_offset` field from `KinesisProperties` struct in `test_single_thread_kinesis_reader` test function - Modify `scan_startup_mode` field in `KinesisProperties` struct - Change accepted values for `scan_startup_mode` to include "timestamp" - Add `timestamp_offset` field to `KinesisProperties` struct Signed-off-by: tabVersion <tabvision@bupt.icu>
|
why does the bot do that |
@tabVersion forgot to tick this "My PR contains breaking changes"
|
Signed-off-by: tabVersion <tabvision@bupt.icu>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
close #12247
as title
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.
allow kinesis
scan.startup.modebetimestampand new option
scan.startup.timestamp.millisas i64please consider removing
scan.startup.sequence_numberandscan.startup.mode = sequence_numberin the docbecause kinesis seq num encodes shard-related info.
lets say
seq_num_0is specified when shard 1, 2, 3 are available, but later some scale op are performed, shard 1 goes down and shard 4 goes up. Specifyseq_num_0can start with shard 2 and 3 but shard 4. The shard 4's executor will goes into a crash loop.cc @neverchanje and @fuyufjh
update: we no longer support
scan.startup.mode = 'sequence_number'and optionscan.startup.sequence_number