-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Improve][Connector-V2-kafka] Support setting read starting offset or time at startup config #3157
Conversation
Please refer to Coding Guide and change your pull request titile. Thx. |
ok |
@Hisoka-X @TyrantLucifer Please help to review |
Map<TopicPartition, ListOffsetsResult.ListOffsetsResultInfo> listOffsets = | ||
getKafkaPartitionLatestOffset(splits.stream().map(KafkaSourceSplit::getTopicPartition).collect(Collectors.toList())); | ||
Map<TopicPartition, Long> listOffsets = | ||
listOffsets(splits.stream().map(KafkaSourceSplit::getTopicPartition).collect(Collectors.toList()), OffsetSpec.earliest()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use OffsetSpec.earliest()
not latest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it was my mistake. I should have used latest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you should add some test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will add e2e tests later
739482e
to
a81b6dd
Compare
… time at startup config
a81b6dd
to
0fc7bf4
Compare
… time at startup config
… time at startup config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this pr, and I will create a pr to add change log.
info-2 = 10 | ||
} | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add changed log
reference https://github.com/apache/incubator-seatunnel/blob/dev/docs/en/connector-v2/source/Redis.md#next-version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok,thank you
Purpose of this pull request
Check list
New License Guide
close #2959