Skip to content
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

changefeedccl: Envelope schema name reflects schema config #60946

Merged

Conversation

HonoreDB
Copy link
Contributor

Fixes a bug where the Avro -envelope schema name, as opposed to subject,
did not honor the schema prefix and full table name options.

Release note (bug fix): Envelope schema in avro registry now honors schema_prefix and full_table_name

@HonoreDB HonoreDB requested review from stevendanna, miretskiy and a team February 22, 2021 21:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@HonoreDB
Copy link
Contributor Author

HonoreDB commented Mar 8, 2021

Ping @stevendanna or @miretskiy, no rush but I think this one got lost. Thanks!

Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we merged part of this in df7be87, but might be worth getting the test addition in still.

Definitely not a blocker, but part of me wonders if we might find a name other than rawTableName that makes it more clear when it is expected that we use the function. Perhaps that is something we could even encode in the type system a bit so that functions expecting a name that has already been prefixed could take a FooName rather than a string.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @HonoreDB and @miretskiy)


pkg/ccl/changefeedccl/encoder.go, line 416 at r1 (raw file):

		opts := avroEnvelopeOpts{afterField: true, beforeField: e.beforeField, updatedField: e.updatedField}
		registered.schema, err = envelopeToAvroSchema(e.rawTableName(row.tableDesc), opts, beforeDataSchema, afterDataSchema)

This isn't a blocker, but I kinda wonder if we could come up with a name other than rawTableName that would make it more clear when we need to call this function.

Fixes a bug where the Avro -envelope schema name, as opposed to subject,
 did not honor the schema prefix and full table name options.

Release note (bug fix): Envelope schema in avro registry now honors schema_prefix and full_table_name
@HonoreDB HonoreDB force-pushed the avro_schema_prefix_in_envelope_name branch from b6ebdc6 to d825112 Compare March 9, 2021 21:33
@HonoreDB
Copy link
Contributor Author

HonoreDB commented Mar 9, 2021


pkg/ccl/changefeedccl/encoder.go, line 416 at r1 (raw file):

Previously, stevendanna (Steven Danna) wrote…

This isn't a blocker, but I kinda wonder if we could come up with a name other than rawTableName that would make it more clear when we need to call this function.

Thanks for the review! I agree, this function name makes even less sense now that it's doing two things. I'll fix in another PR so as to keep the backports aligned.

@HonoreDB
Copy link
Contributor Author

bors r=[stevendanna]

@craig
Copy link
Contributor

craig bot commented Mar 11, 2021

Build succeeded:

@craig craig bot merged commit d01a9e5 into cockroachdb:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants