refuse unused WITH options in CONFLUENT SCHEMA REGISTRY #10129
Conversation
guswynn
force-pushed
the
ccsr_options
branch
2 times, most recently
from
02b4ab8
to
64d2a94
Compare
src/sql/src/kafka_util.rs
Outdated
| &[ | ||
| Config::string("username"), | ||
| Config::string("password"), | ||
| // An old migration added this field in some avro cases, so we remove it here |
There was a problem hiding this comment.
actually, this comment might be wrong, but I couldn't figure out the true reason
There was a problem hiding this comment.
We should definitely figure out the real reason before merging this. I'm happy to help. What's the test that fails without this?
src/sql/src/plan/statement/ddl.rs
Outdated
| @@ -1018,6 +1018,16 @@ pub fn plan_create_source( | |||
| })) | |||
| } | |||
|
|
|||
| fn ensure_ccsr_options(ccsr_with_options: &BTreeMap<String, Value>) -> Result<(), anyhow::Error> { | |||
There was a problem hiding this comment.
I think it might make sense to move this function next to the with_options! macro in sql::normalize and rename it to ensure_empty_options(options: &Btreemap<String, Value>, for: &str) and then call it like ensure_empty_options(&ccsr_options, "CONFLUENT SCHEMA REGISTRY")?;.
| } => { | ||
| if let Some(CsrSeedCompiledOrLegacy::Compiled(CsrSeedCompiled { key, value })) = | ||
| seed | ||
| { | ||
| let mut ccsr_with_options = normalize::options(&ccsr_options); | ||
|
|
||
| // We validate here instead of in purification, to match the behavior of avro |
There was a problem hiding this comment.
which avro behavior exactly is this matching?
There was a problem hiding this comment.
validating that there are no empty options, even though the protobuf decoder doesn't use this ccsr config
|
I do think that this deserves a release note. Users who were passing invalid options will get an error, which is user-visible. |
doc/user/content/release-notes.md
Outdated
| - **Breaking change.** `CONFLUENT SCHEMA REGISTRY` sections | ||
| of `CREATE SOURCE` and `CREATE SINK` now reject unknown parameters. | ||
| (Example: [Confluent Schema Registry options](/sql/create-source/avro-kafka#confluent-schema-registry-options)). | ||
|
|
There was a problem hiding this comment.
This slipped into the v0.17.0 section but needs to go in the unreleased section above!
src/sql/src/kafka_util.rs
Outdated
| &[ | ||
| Config::string("username"), | ||
| Config::string("password"), | ||
| // An old migration added this field in some avro cases, so we remove it here |
There was a problem hiding this comment.
We should definitely figure out the real reason before merging this. I'm happy to help. What's the test that fails without this?
src/sql/src/normalize.rs
Outdated
| /// `WITH` options are all real, used options | ||
| pub(crate) fn ensure_empty_options<V>( | ||
| with_options: &BTreeMap<String, V>, | ||
| r#for: &str, |
There was a problem hiding this comment.
Using a raw ident is def nonstandard practice. Elsewhere in sql we call this sort of parameter name or ctx.
|
@benesch I think this update should cover all your review! |
| // An old migration | ||
| // (https://github.com/MaterializeInc/materialize/blob/c402917b7078a14bc0d0d6330b9c9b177aa73e47/src/coord/src/catalog/migrate.rs#L362) | ||
| // adds this field to kafka-avro WITH options, though now in the CSR case, | ||
| // the field is unread and fixed to `true` |
There was a problem hiding this comment.
That was just an error in the migration, no? Like this parameter has no meaning here and has never had meaning, right? Too bad. If that's true we should write another migration to fix it. Doesn't need to block this though since this is still a huge net win. Can you file a followup ticket though?
There was a problem hiding this comment.
I am not sure if it never had meaning, but yeah, now It appears to not have meaning...I will make an issue
also, I removed the username and password extraction, because I don't actually think those exist except in docs?
Currently, if a
CONFLUENT SCHEMA REGISTRYWITHoption is unused, nothing happens. This leads to more complex migrations, and does not match the behavior of kafka options.This diff validates that only accepted options are accepted, and errors otherwise. It uses a similar technique as the kafka options, passing
&mutout parameters and removing options instead ofget'ing themthe
generate_ccsr_client_configfunction is used in a couple places, but we only validate no unused options inplan_create_sourceandplan_create_sink, to match how kafka option validation works, which is why validation happens outside that functionI am not sure where and how
confluent_wire_formatcomes from, so where to strip that is not clear to meMotivation
Checklist