fix(sink): make input_specs attributes optional and complete - #224
Conversation
Every nested attribute of pulsar_sink's input_specs block was Required, so tuning receiver_queue_size on one topic forced the user to also name a schema_type and a serde_class_name. That is not just inconvenient: Pulsar rejects a spec that sets both, so the schema demanded a combination the broker refuses. Make all of them Optional except the topic key, which is the map key and so genuinely required. Add the ConsumerConfig fields that had no attribute: pool_messages, schema_properties and consumer_properties. Add validation for the two rules the schema cannot express, mirroring the approach used for pulsar_function's input_specs: - topics must be unique, since they are the map key and two blocks sharing one would silently collapse to whichever the set iterated last - schema_type and serde_class_name cannot both be set, matching what SinkConfigUtils enforces server-side, so the error arrives at plan time rather than apply time Empty schema_properties and consumer_properties maps are omitted from the request, and are only written into state when the broker returns something for them, so an empty map does not read as configuration the user never wrote. Fixes streamnative#218 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ields TestImportExistingSink asserts the exact number of flattened state attributes on an imported sink. Adding pool_messages, schema_properties and consumer_properties to input_specs takes that count from 30 to 33, so the assertion had to move with it. Pull the number into a named constant and say what it counts, since a bare literal gives no clue why it changes when the schema does - which is how this was missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed a fix for the acceptance test failure.
My original verification was too narrow: I ran One thing worth flagging for the record: during that investigation I saw Also unrelated to this change, but visible in the failed run: the workflow's tmate debug step keeps the job alive for ~15 minutes after a failure and fills the log tail with SSH banners, which buries the actual assertion. Worth a separate look if others hit the same confusion. |
|
Pushed Changes:
Regression coverage now includes explicit zero presence, plan-time validation, legacy overlap filtering, in-place consumer updates, topology replacement, import normalization, and real broker read-back. Validation:
|
david-streamlio
left a comment
There was a problem hiding this comment.
Thanks for picking this up, @freeznet — the overlap dedup and the receiver_queue_size = 0 presence handling are real fixes, and asserting the resource ID is unchanged in TestSinkUpdate is exactly the right way to prove the ForceNew rework avoids replacement.
I went through caf2c36 in detail. Build, go vet, the unit tests and Provider().InternalValidate() all pass locally; I have not run the acceptance tests (no live cluster here).
One item I would like to resolve before this merges, plus two behaviour changes worth making explicit:
schema_propertieswas removed. It is one of the three attributes this PR exists to add, andConsumerConfig.SchemaPropertiesis still live upstream. Details inline — this is the only one I would call blocking.- Refresh no longer writes back
inputs/topics_pattern/custom_serde_inputs/custom_schema_inputs, so drift on those is no longer detected, and imported state carries noinputsat all. receiver_queue_sizeis now always transmitted rather than omitted when unset.
Happy to push fixes for any of these myself if you would rather not carry them — just say which.
|
@maxsxu ready for a re-review when you have a moment — flagging explicitly because the review request no longer shows on this PR: both your approval and @freeznet's were dismissed by the two pushes in State as of now:
Nothing outstanding from my side. |
Fixes #218
Motivation
Every nested attribute of
pulsar_sink'sinput_specsblock isRequired:resourceSinkInputSpecsSubsetTopicKey: {Type: schema.TypeString, Required: true}, resourceSinkInputSpecsSubsetSchemaTypeKey: {Type: schema.TypeString, Required: true}, resourceSinkInputSpecsSubsetSerdeClassNameKey: {Type: schema.TypeString, Required: true}, resourceSinkInputSpecsSubsetIsRegexPatternKey: {Type: schema.TypeBool, Required: true}, resourceSinkInputSpecsSubsetReceiverQueueSizeKey: {Type: schema.TypeInt, Required: true},So tuning
receiver_queue_sizeon one topic forces the user to also name aschema_typeand aserde_class_name. That is worse than inconvenient:SinkConfigUtils.doJavaChecks()rejects a spec that sets both ("Only one of schemaType or serdeClassName should be set in inputSpec"), so the schema requires a combination the broker refuses.utils.ConsumerConfigalso carriesPoolMessages,SchemaPropertiesandConsumerProperties, none of which had an attribute.Modifications
Optionalexceptkey, which is theInputSpecsmap key and so genuinely required.pool_messages,schema_propertiesandconsumer_properties.validateSinkInputSpecsfor the two rules the schema cannot express, mirroring the approach taken forpulsar_function'sinput_specsin feat(function): expose input_specs for per-topic consumer config #219:TypeSethashes the whole nested object, so two blocks with the samekeybut different queue sizes are both valid set elements and then collapse to whichever the map iteration wrote last. Rejecting this at plan time avoids an ambiguous request and perpetual drift.schema_type/serde_class_nameexclusivity, matching the server, so the error arrives at plan time rather than apply time.receiver_queue_size >= 0validation and mark itComputed, since the broker supplies a default.schema_properties/consumer_propertiesmaps are omitted from the request and only written into state when the broker returns something, so an empty map does not read as configuration the user never wrote.Backward compatibility — worth your read. Relaxing
RequiredtoOptionalis not breaking for existing configurations: a config that sets all five attributes keeps working unchanged. It does mean state written by an older provider version carries explicit zero values ("",false,0) where a user might now omit the attribute. I did not add a state upgrader, on the grounds that those zero values are what the schema previously forced and remain valid. Say the word if you would rather have one.The attribute names are left as they are —
key,serde_class_name,is_regex_pattern— rather than aligned withpulsar_function's newerinput_specsblock, since renaming them would be genuinely breaking.Verifying this change
This change added tests and can be verified as follows:
resource_pulsar_sink_unit_test.go: a spec that sets onlyreceiver_queue_sizemarshals correctly with no schema or serde (the case the issue is about); the three new fields round-trip; and the validation rejects duplicate topics and the schema/serde combination while accepting queue-size-only and distinct-topic specs.TestSinkandTestSinkUpdatepass against Pulsar 4.0.3, confirming existing sink configurations are unaffected.make test,golangci-lintandgo generate ./...are all clean.Documentation
Check the box below.
Need to update docs?
doc-requiredno-need-docdocdocs/resources/sink.mdregenerated; the nested block now shows one required and seven optional attributes, each with a description.