changefeedccl: reduce direct access to changefeeddetails #83153
Conversation
|
This change is |
HonoreDB
force-pushed
the
marker_types
branch
2 times, most recently
from
b570bd7 to
fc01403
Compare
| StatementTimeName StatementTimeName | ||
| } | ||
|
|
||
| type StatementTimeName string |
There was a problem hiding this comment.
lint would probably complain about lack of comments on exported symbols.
(even if it won't, we probably should have those).
|
|
||
| type StatementTimeName string | ||
| type WatchedTables map[descpb.ID]StatementTimeName | ||
| type Targets []Target |
There was a problem hiding this comment.
Do we imagine having methods on Targets? If not, perhaps we should use []Target at the call sites?
pkg/ccl/changefeedccl/changefeed.go
Outdated
| @@ -22,6 +23,75 @@ import ( | |||
| "github.com/cockroachdb/cockroach/pkg/util/uuid" | |||
| ) | |||
|
|
|||
| // Changefeed provides a version-agnostic wrapper around jobspb.ChangefeedDetails. | |||
| type Changefeed struct { | |||
There was a problem hiding this comment.
I would mildly prefer not to call this Changefeed, but something more specific.
Maybe one one of (CDC|Feed|Changefeed)(Config|Cfg)?
pkg/ccl/changefeedccl/changefeed.go
Outdated
| Targets changefeedbase.Targets | ||
| } | ||
|
|
||
| func makeChangefeedFromJobDetails(d jobspb.ChangefeedDetails) Changefeed { |
There was a problem hiding this comment.
Function comment por favore.
| @@ -389,7 +389,9 @@ func (s *bufferSink) Dial() error { | |||
| // TODO (zinger): Make this a tuple or array datum if it can be | |||
| // done without breaking backwards compatibility. | |||
| func (s *bufferSink) getTopicDatum(t TopicDescriptor) *tree.DString { | |||
| return s.alloc.NewDString(tree.DString(strings.Join(t.GetNameComponents(), "."))) | |||
| name, components := t.GetNameComponents() | |||
| strs := append([]string{string(name)}, components...) | |||
There was a problem hiding this comment.
a bit sad that we have to alloc new array every time...
Do you think we can/should special case when we have nil components?
HonoreDB
force-pushed
the
marker_types
branch
2 times, most recently
from
c9cb790 to
f3bbeec
Compare
There was a problem hiding this comment.
TFTR!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @livlobo and @miretskiy)
pkg/ccl/changefeedccl/changefeed.go line 27 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
I would mildly prefer not to call this
Changefeed, but something more specific.
Maybe one one of(CDC|Feed|Changefeed)(Config|Cfg)?
Changed to ChangefeedConfig.
pkg/ccl/changefeedccl/changefeed.go line 44 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
Function comment por favore.
Done.
pkg/ccl/changefeedccl/sink.go line 393 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
a bit sad that we have to alloc new array every time...
Do you think we can/should special case when we have nil components?
Done.
pkg/ccl/changefeedccl/changefeedbase/target.go line 24 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
lint would probably complain about lack of comments on exported symbols.
(even if it won't, we probably should have those).
Done.
pkg/ccl/changefeedccl/changefeedbase/target.go line 26 at r1 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
Do we imagine having methods on
Targets? If not, perhaps we should use[]Targetat the call sites?
I am imagining having methods on Targets in a future PR, yeah, I think probably the thing to do is consolidate it and WatchedTables into one type that you can efficiently access in different ways (EachTarget, EachTable, FindStatementTimeNameByID).
Directly referencing fields on the jobs proto makes inter-version compatibility harder. This PR takes a first step toward making most methods use changefeedbase objects instead. Release note: None
|
bors r=[miretskiy] |
|
Build failed: |
|
bors retry |
|
Build succeeded: |
Directly referencing fields on the jobs proto makes
inter-version compatibility harder.
This PR takes a first step toward making most methods use
changefeedbase objects instead.
Fixes #82342.
Release note: None