-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
release-23.1: changefeedccl: add nil checking for avroDataRecord.refreshTypeMetadata #119746
release-23.1: changefeedccl: add nil checking for avroDataRecord.refreshTypeMetadata #119746
Conversation
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jayshrivastava, @rharding6373, @stevendanna, and @wenyihu6)
pkg/ccl/changefeedccl/encoder_test.go
line 1115 at r1 (raw file):
cdcTest(t, testFn, feedTestForceSink("cloudstorage")) }
nit: extra newline
pkg/ccl/changefeedccl/encoder_test.go
line 1187 at r1 (raw file):
} }
nit: extra newline
4638ab3
to
1da9d17
Compare
Friendly ping on the PR review here : ) Hoping to merge it before the next branch cut. |
@stevendanna do you mind taking a quick look at this PR? Backporting requires stamps from 2 TLs. |
1da9d17
to
22614dc
Compare
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.
Thanks for the explanation!
This is a clear bug fix for a bug that causes a node panic, which I view as serious.
Previously, the "after" field was populated across all envelope formats. However, in commit c00bf0f, row data was moved to the “after” field for envelope = wrapped and to “record” field for other envelope formats. But the commit did not add a nil check for registered.schema.after which is only populated for wrapped envelope. registered.schema.after is now nil for other envelope formats, and calling refreshTypeMetadata on it could lead to node panics. In addition, this commit did not add the logic calling refreshTypeMetadata on the registered.schema.record for other envelope formats. This patch addresses the issue by adding a defensive nil check before invoking refreshTypeMetadata and adding refreshTypeMetadata for the record field when it's set to match the original intents of the commit. Fixes: cockroachdb#119428 Release note: None
22614dc
to
1880dc6
Compare
Backport 1/1 commits from #119639.
/cc @cockroachdb/release
Previously, the "after" field was populated across all envelope formats.
However, in commit c00bf0f, row data was moved to the “after” field for envelope
= wrapped and to “record” field for other envelope formats. But the commit did
not add a nil check for registered.schema.after which is only populated for
wrapped envelope.
registered.schema.after
is now nil for other envelopeformats, and calling
refreshTypeMetadata
on it could lead to node panics. Inaddition, this commit did not add the logic calling
refreshTypeMetadata
on theregistered.schema.record for other envelope formats. This patch addresses the
issue by adding a defensive nil check before invoking
refreshTypeMetadata
andadding
refreshTypeMetadata
for the record field when it's set to match theoriginal intents of the commit.
Fixes: #119428
Release note: None
Release justification: low risk bug fix