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.
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
feat: add kafka backfill frontend #15602
feat: add kafka backfill frontend #15602
Changes from 25 commits
9c44201
55fde18
2285c1f
6aa8502
4c7ae69
2daa0a8
8d996f4
82afb30
5abbb11
fef7611
8805238
d028469
cc8cb41
4f850de
31b34f0
87c7cad
27db3f6
9f8f533
78a783f
06027ca
fa521f0
58034cc
18ea279
658cb52
231b93c
0339b38
17e3779
2e5c1c0
144d7ae
e174c12
98cfb72
f169dec
aafba45
a3accdb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Deprecating
cdc_source_job
and addinghas_streaming_job
will break backward compability of existing CDC jobs, right?Why not renaming
cdc_source_job
tohas_streaming_job
?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.
has_streaming_job
is misleading, too. I think we should mention source to indicate what it controls happens whencreate source
, eg.source_has_streaming_job
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.
Changing the field name could be breaking under SQL meta backend. 😐 Not sure why it's not linted.
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.
This is indeed the change I made. Number
13
is renamed.Indeed. 😕 Need to come up with another plan.
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.
do you already have one? Keeping it or introducing another one dedicated to this case both 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.
New plan:
cdc_source_job
. New & old CDC sources will both set this field. This field is only used to setrequire_singleton
is_shared
tooptional
(explicit presence). Old CDC sources will have this field set toNone
. New CDC sources will set toSome(true)
.is_shared_compatibile
to testcdc_source_job
first. TBH, this is a little awkward. Better ideas are welcome.optional
,(cdc_source_job, is_shared) = (true,false)
can also work. But I feel this can make it (just slightly) less error-prone.Old plan:
cdc_source_job
tois_shared
. This is more elegant, but breaks JSON compatibility for SQL meta backend.is_distributed
to setrequire_singleton
in fragmenter for CDC source.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 off-topic... I think storing protobuf into JSON sounds bad.
https://protobuf.dev/programming-guides/dos-donts/#text-format-interchange