changefeedccl: Bring changefeed transformations out of preview #94806
Conversation
|
This change is |
|
@HonoreDB Only top 2 commits. |
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, 11 of 11 files at r2, 17 of 17 files at r3, 16 of 16 files at r4, 11 of 11 files at r5, 2 of 2 files at r6, 11 of 11 files at r7, 7 of 7 files at r8, 3 of 11 files at r9.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @samiskin)
pkg/ccl/changefeedccl/metrics.go line 387 at r8 (raw file):
metaChangefeedFilteredMessages := metric.Metadata{ Name: "changefeed.filtered_messages", Help: "Messages filtered out by all feeds",
Maybe specify "filtered out in memory" in the description and I think also in the name. I assume this doesn't include "messages" that got filtered out by the range constraints before they even became messages which is going to be confusing to the user (if you do N writes, EmittedMessages+FilteredMessages can be less than N).
pkg/ccl/changefeedccl/cdceval/compat.go line 29 at r9 (raw file):
// Replace this call with a call to row_to_json((cdc_prev).*) if n, ok := t.Func.FunctionReference.(*tree.UnresolvedName); ok && n.Parts[0] == "cdc_prev" { rowToJSON := &tree.FuncExpr{
could this be a singleton?
pkg/jobs/jobspb/jobs.proto line 936 at r8 (raw file):
string select = 10; sessiondatapb.SessionData session_data = 11 [(gogoproto.nullable) = false];
I'm not sure this change is backwards-compatible, is it meant to be? Non-nullable fields are serialized differently I think.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/changefeedccl/metrics.go line 387 at r8 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
Maybe specify "filtered out in memory" in the description and I think also in the name. I assume this doesn't include "messages" that got filtered out by the range constraints before they even became messages which is going to be confusing to the user (if you do N writes, EmittedMessages+FilteredMessages can be less than N).
i'd rather not add memory bit; but you do bring up a good point; alas, w/out explain statement, there
is no way to tell how many were filtered. I've expanded help message a bit to at least mention this.
pkg/ccl/changefeedccl/cdceval/compat.go line 29 at r9 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
could this be a singleton?
I'm not sure tbh... But I'm honestly not worried about this.
pkg/jobs/jobspb/jobs.proto line 936 at r8 (raw file):
Previously, HonoreDB (Aaron Zinger) wrote…
I'm not sure this change is backwards-compatible, is it meant to be? Non-nullable fields are serialized differently I think.
Sure; but I think it's fine -- session data was added in prior PRs -- and arguably, I should have
added version gate, but I didn't. Anyway, that code was never released, so I think it's fine to make this change now.
|
On the obs-infra side, the new metric & new addition to the chart catalog LGTM |
|
@HonoreDB -- want to make sure you saw recent updates... |
Add metric to keep track of the number of filtered messags. Add debug logging to cdc evaluation library. Epic: CRDB-17161 Release note: None
Add a mechanism to migrate changefeed expressions created in 22.2 clusters to be compatible with recently introduced changes. Old changefeed expressions are transparently rewritten, and the changefeed job record automatically updated once cluster upgrade has been finalized. A warning message may be logged if the rewritten expression is inefficient. The automatic update might still fail. The end users are encourraged to closely monitor the health of the changefeeds during the upgrade, and recreate any failed changefeeds as needed. Epic: CRDB-17161 Release note (enterprise change): Changefeed transformations introduced in 22.2 release in preview mode are no longer experimental. This feature can now be considered to be fully production ready.
There was a problem hiding this comment.
Reviewed 46 of 46 files at r10, 7 of 43 files at r11.
Reviewable status:
pkg/ccl/changefeedccl/changefeed_stmt.go line 1400 at r11 (raw file):
// Job record upgraded. Return transient error to reload job record and retry. if newExpression == oldExpression {
I think safer to compare tree.AsString before and after than directly compare pointers?
pkg/ccl/changefeedccl/metrics.go line 387 at r8 (raw file):
Previously, miretskiy (Yevgeniy Miretskiy) wrote…
i'd rather not add memory bit; but you do bring up a good point; alas, w/out explain statement, there
is no way to tell how many were filtered. I've expanded help message a bit to at least mention this.
I agree there's no way to efficiently tell right now. I'm not sure "the range constraints" communicates this to a non-roacher, would "due to primary key constraints in WHERE clauses" be reasonably accurate?
done. |
I think it's fine; walk methods compare pointers when they need to determine if something changed. |
|
Argh; accidentally merged w/out updates to doc string. |
Add a mechanism to migrate changefeed expressions
created in 22.2 clusters to be compatible with
recently introduced changes.
Old changefeed expressions are transparently rewritten,
and the changefeed job record automatically updated
once cluster upgrade has been finalized.
A warning message may be logged if the rewritten expression
is inefficient.
The automatic update might still fail. The end users
are encourraged to closely monitor the health of the
changefeeds during the upgrade, and recreate any failed
changefeeds as needed.
Epic: CRDB-17161
Release note (enterprise change): Changefeed transformations
introduced in 22.2 release in preview mode are no longer
experimental. This feature can now be considered
to be fully production ready.