To come back about a point Chris made: 1. Instead of the new "ConfigDef config(Map<String, String> props)" method, > what would you think about adopting a similar approach as the framework > uses with connectors, by adding a "Config validate(Map<String, String> > props)" method that can perform custom validation outside of what can be > performed by the ConfigDef's single-property-at-a-time validation? It may > be a little heavyweight for use with this particular SMT, but it'd provide > more flexibility for other SMT implementations and would mirror an API that > developers targeting the framework are likely already familiar with. >
The validate() + config() approach taken for Connectors doesn't quite work for Transformations. The default Connector.validate() basically just calls `config().validate(connectorConfigs)` and returns a Config. Separately the AbstractHeader also calls `config()` and uses the Config and ConfigDef to build the ConfigInfos. The contract is not really defined well enough in the javadoc, but this means a connector can use validate() to build the ConfigDef which it will return from config(). For Transformations we don't really want validate() to perform validation at all since ultimately the transformations config will be embedded in the connector's and will be validated by the connector itself. It wouldn't be harmful if it _did_ perform validation, just unnecessary. But having a validate() method that ought not to validate() seems like a recipe for confusion. Also problematic is that there's no use for the Config returned from Transformations.validate(). So I'm not convinced that the superficial similarity really gains anything. Kind regards, Tom On Mon, Apr 6, 2020 at 2:29 PM Tom Bentley <tbent...@redhat.com> wrote: > Hi, > > Hi all, > > Thanks for the discussion so far. > > It seems a bit weird to me that when configuring the Conditional SMT with > a DSL you would use a concise, intuitive DSL for expressing the condition, > but not for the transforms that it's guarding. It also seems natural, if > you support this for conditionally applying SMTs, that you'd soon want to > support the same thing for a generic filter transformer. Then, if you can > configure a filter transformation using this DSL, it becomes odd that you > can't do this for mapping transformations. I think it would be a mistake to > go specifying an expression language for conditions when really that might > just be part of a language for transformations. > > I think it would be possible today to write an SMT which allowed you to > express the transformation in a DSL. Concretely, it's possible to imagine a > single transformation taking a DSL something like this: > > compose( > Flatten.Key(delimiter: ':'), > If(condition: TopicMatches(pattern: "blah-.*"), > then: Flatten.Value(delimiter: '/'))) > > All I've really done here, beyond what was already proposed, is rename > Conditional to If, imagine a couple more bits of syntax > (SMTs being constructed by class name and named argument invocation syntax > to support SMT constructors) and add a higher level compose() function for > chaining SMTs (which could easily be replaced with brace delimited blocks). > > That may be a discussion we should have, but I think in _this_ KIP we > should focus on the conditional part, since the above example hopefully > shows that it would be possible to reuse it in a DSL if there was appetite > for that. > > With that in mind, and since my original suggestion for an abbreviated > config syntax didn't appeal to people, I propose that we stick with the > existing norms for configuring this. > The previous example would look like this: > > transformation: flattenKey,if > transformation.flattenKey.type: Flatten.Key > transformation.flattenKey.delimiter: : > transformation.if.type: If > transformation.if.condition.type: TopicMatches > transformation.if.condition.pattern: blah-.* > transformation.if.then: flattenValue > transformation.if.then.flattenValue.type: Flatten.Value > transformation.if.then.flattenValue.delimiter: / > > Also, I'm inclined to think we should stick to just supporting > TopicMatches and Not, since we've not identified an actual need for > 'has-header', 'or' and 'and'. > > An example of the usage of Not would look like this: > > transformation: flattenKey,if > transformation.flattenKey.type: Flatten.Key > transformation.flattenKey.delimiter: : > transformation.if.type: If > transformation.if.condition.type: Not > transformation.if.condition.operand: startsWithBlah > transformation.if.condition.operand.startsWithBlah.type: TopicMatches > transformation.if.condition.operand.startsWithBlah.pattern: blah-.* > transformation.if.then: flattenValue > transformation.if.then.flattenValue.type: Flatten.Value > transformation.if.then.flattenValue.delimiter: / > > Hopefully we can agree that this allows the conditional SMT part to make > progress without getting bogged down. > > Thoughts? If this seems broadly reasonable, I'll update the KIP. > > Kind regards, > > Tom > > On Fri, Apr 3, 2020 at 5:55 PM Gunnar Morling <gun...@hibernate.org> > wrote: > >> Hi all, >> >> Thanks a lot for this initiative, Tom! >> >> To shed some light, the use case where this first came up, were issues >> we saw with SMTs being applied to the different topics produced by the >> Debezium change data capture connectors. There are different kinds of >> topics (for change data, schema history, heartbeats etc.) and the >> record structure to expect may vary between those. Hence we saw issues >> with SMTs like ExtractField, which for instance only should be applied >> to all change data topics but not the other ones. >> >> I like the overall approach; for Debezium's purposes, the simple topic >> matching and negation operators would be sufficient already. I agree >> with Chris and would prefer one single condition attribute, which >> contains a single condition or potentially a logical expression with >> not, and, etc. I think it's less ambiguous, in particular when it >> comes to ordering of the different conditions and determining their >> precedence. >> >> Would love to see this feature in one or another way in Connect. >> >> Best, >> >> --Gunnar >> >> >> >> Am Do., 2. Apr. 2020 um 18:48 Uhr schrieb Tom Bentley < >> tbent...@redhat.com>: >> > >> > Hi Chris and Sönke, >> > >> > Using the numbering from Chris's email... >> > >> > 1. That's a good point, I'll see what is needed to make that work. >> > >> > 2. I'm happy enough to add support for "and" and "or" as part of this >> KIP >> > if people can see a need for it. >> > >> > In a similar vein, I was wondering about whether it would be worthwhile >> > having the equivalent of an "else" clause (what's in the KIP is >> basically >> > an "if" statement). Without support for "else" I think people would >> often >> > need two conditionals, with the condition of one being the negation of >> the >> > condition of another. >> > >> > 3. I can see the attraction of an expression language. The pros include >> > being terse and familiar to programmers and potentially very flexible if >> > that's needed in the future. I had a play and implemented it using ANTLR >> > and it's not difficult to write a grammar and implement the functions >> we've >> > already discussed and get decent error messages when the expression is >> > malformed. So on the one hand I quite like the idea. On the other hand >> it >> > feels like overkill for the use cases that have actually been >> identified so >> > far. >> > >> > @Sönke what do you make of the expression language idea? >> > >> > Kind regards, >> > >> > Tom >> > >> > On Wed, Apr 1, 2020 at 9:49 PM Christopher Egerton <chr...@confluent.io >> > >> > wrote: >> > >> > > Hi Tom, >> > > >> > > This looks great and I'd love to see the out-of-the-box SMTs become >> even >> > > more powerful with the improvements you've proposed! Got a few >> remarks and >> > > would be interested in your thoughts: >> > > >> > > 1. Instead of the new "ConfigDef config(Map<String, String> props)" >> method, >> > > what would you think about adopting a similar approach as the >> framework >> > > uses with connectors, by adding a "Config validate(Map<String, String> >> > > props)" method that can perform custom validation outside of what can >> be >> > > performed by the ConfigDef's single-property-at-a-time validation? It >> may >> > > be a little heavyweight for use with this particular SMT, but it'd >> provide >> > > more flexibility for other SMT implementations and would mirror an >> API that >> > > developers targeting the framework are likely already familiar with. >> > > 2. The possibility for adding the logical operators "and" and "or" is >> > > mentioned, but only as a potential future change and not one proposed >> by >> > > this KIP. Why not include those operators sooner rather than later? >> > > 3. The syntax for named conditions that are then referenced in logical >> > > operators is... tricky. It took me a few attempts to grok the example >> > > provided in the KIP after reading Sönke's question about the example >> for >> > > negation. What about a more sophisticated but less verbose syntax that >> > > supports a single configuration for the condition, even with logical >> > > operators? I'm thinking something like >> > > "transforms.conditionalExtract.condition: not(has-header:my-header)" >> > > instead of the "transforms.conditionalExtract.condition: >> not:hasMyHeader" >> > > and "transforms.conditionalExtract.condition.hasMyHeader: >> > > has-header:my-header" properties. If support for a logical "and" is >> added, >> > > this could then be expanded to something like >> > > "transforms.conditionalExtract.condition: and(has-header(my-header), >> > > not(topic-matches(my-prefix-.*)))". There would be additional >> complexity >> > > here with the need to escape parentheses and commas that are intended >> to be >> > > treated literally (as part of a header name, for example) instead of >> as >> > > part of the syntax for the condition itself, but a little additional >> > > complexity for edge cases like that may be warranted if it heavily >> reduces >> > > complexity for the common cases. The rationale for the proposed >> > > parentheses-based syntax here instead of what's mentioned in the KIP >> > > (something like "and: <condition1>, <condition2>") is to help with >> > > readability; we probably wouldn't need that with the approach of >> naming >> > > conditions via separate properties, but things may get a little nasty >> with >> > > literal conditions included there, especially with the possibility of >> > > nesting. Finally, the shift in syntax here should make it easier to >> handle >> > > cases like the header value matching brought up by Sönke; it might >> look >> > > something like "header-matches(header-name, header-value-pattern)". >> > > >> > > Cheers, >> > > >> > > Chris >> > > >> > > On Wed, Apr 1, 2020 at 9:00 AM Tom Bentley <tbent...@redhat.com> >> wrote: >> > > >> > > > Hi Sönke, >> > > > >> > > > Thanks for taking a look. >> > > > >> > > > Let me answer in reverse order, since I think it might make more >> sense >> > > that >> > > > way... >> > > > >> > > > Also, while writing that, I noticed that you have a version with and >> > > > > without "name" for your transformation in the KIP: >> > > > > >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> > > has-header:my-header >> > > > > and >> > > > > transforms.conditionalExtract.condition: has-header:my-header >> > > > > >> > > > >> > > > The example >> > > > transforms.conditionalExtract.condition: has-header:my-header >> > > > is a "has-header" condition (the prefix of the config value), which >> will >> > > > match records with a "my-header" header (given in the suffix). >> > > > >> > > > The other example given is: >> > > > transforms.conditionalExtract.condition: not:hasMyHeader >> > > > transforms.conditionalExtract.condition.hasMyHeader: >> > > > has-header:my-header >> > > > The root of the condition is a "not" condition (the prefix of the >> value >> > > for >> > > > the transforms.conditionalExtract.condition key) of another named >> > > condition >> > > > called "hasMyHeader" (the suffix). Any name could be used for the >> other >> > > > condition. That other condition is configured at >> > > > "transforms.conditionalExtract.condition.<conditionName>". That >> condition >> > > > is a "has-header" condition (the prefix), which will match records >> with a >> > > > "my-header" header (given in the suffix). So the "has-header:" >> condition >> > > > type would always require a suffix, as would the "not:" condition >> type. >> > > > Hypothetically you could have a "true" condition type (which would >> not >> > > > require a suffix), and the hypothetical binary conditions "and:" and >> > > "or:" >> > > > would require a pair of other condition names. >> > > > >> > > > So what's proposed is a scheme for encoding conditions where the >> > > condition >> > > > type is the prefix of the value of some "....condition" config key, >> and >> > > the >> > > > optional suffix provides parameters for the condition. This makes >> those >> > > > parameters a bit inflexible, but is relatively succinct. >> > > > >> > > > This leads on to your first point. You're right that use cases might >> > > appear >> > > > which need other conditions, and we should make it flexible enough >> to be >> > > > able to cope with future use cases. On the other hand, I was >> concerned >> > > that >> > > > we end up with something which is quite complicated to configure. >> (There >> > > > comes a point where it might makes more sense for the user to write >> their >> > > > own SMT). >> > > > >> > > > Just of the top of my head it might look like: >> > > > > >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> type:has-header >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> > > > header-name:my-header >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> > > field-value:my-value >> > > > > >> > > > >> > > > That won't work because the format is basically a >> Properties/Map<String, >> > > > String> and what you've suggested has duplicate keys. >> > > > >> > > > One thing I did briefly consider what the ability to treat >> conditions as >> > > > Configurable objects in their own right (opening up the possibility >> of >> > > > people supplying their own Conditions, just like they can supply >> their >> > > own >> > > > SMTs). That might be configured something like this: >> > > > >> > > > transforms.conditionalExtract.condition: not >> > > > transforms.conditionalExtract.condition.not.type: Not >> > > > transforms.conditionalExtract.condition.not.negated: foo >> > > > transforms.conditionalExtract.condition.foo.type: >> HasHeaderWithValue >> > > > transforms.conditionalExtract.condition.foo.header: my-header >> > > > transforms.conditionalExtract.condition.foo.value: my-value >> > > > >> > > > I didn't propose that I couldn't see the use cases to justify this >> kind >> > > of >> > > > complexity, especially as the common case would surely be matching >> > > against >> > > > topic name (to be honest I wasn't completely convinced by the need >> for >> > > > "has-header"). In the current version of the KIP that's just >> > > > >> > > > transforms.conditionalExtract.condition: topic-matches: >> my-prefix-.* >> > > > >> > > > but using the more flexible scheme that would require something >> more like >> > > > this: >> > > > >> > > > transforms.conditionalExtract.condition: bar >> > > > transforms.conditionalExtract.condition.bar.type: TopicMatch >> > > > transforms.conditionalExtract.condition.bar.pattern: >> my-prefix-.* >> > > > >> > > > If people know of use cases which would justify more >> sophistication, I'm >> > > > happy to reconsider. >> > > > >> > > > Thanks again for taking a look! >> > > > >> > > > Tom >> > > > >> > > > On Wed, Apr 1, 2020 at 2:05 PM Sönke Liebau >> > > > <soenke.lie...@opencore.com.invalid> wrote: >> > > > >> > > > > Hi Tom, >> > > > > >> > > > > sounds useful to me, thanks for the KIP! >> > > > > The only thought that I had while reading was that this will >> probably >> > > > raise >> > > > > questions about more involved conditions fairly quickly. For >> example >> > > the >> > > > > "has-header" will cause an appetite for conditions like >> > > > > "this-header-has-that-value". >> > > > > This would necessitate two parameters to be passed into the >> condition, >> > > > > which I think is not currently included in the KIP. I am not >> saying add >> > > > > this now, but might it make sense to discuss a concept of how that >> > > might >> > > > > look now, to avoid potential changes later on. >> > > > > >> > > > > Just of the top of my head it might look like: >> > > > > >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> type:has-header >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> > > > header-name:my-header >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> > > field-value:my-value >> > > > > >> > > > > >> > > > > Also, while writing that, I noticed that you have a version with >> and >> > > > > without "name" for your transformation in the KIP: >> > > > > >> > > > > transforms.conditionalExtract.condition.hasMyHeader: >> > > has-header:my-header >> > > > > and >> > > > > transforms.conditionalExtract.condition: has-header:my-header >> > > > > >> > > > > >> > > > > Is this intentional and the name is optional? >> > > > > >> > > > > >> > > > > Best regards, >> > > > > >> > > > > Sönke >> > > > > >> > > > > >> > > > > >> > > > > On Wed, 1 Apr 2020 at 11:12, Tom Bentley <tbent...@redhat.com> >> wrote: >> > > > > > >> > > > > > Does anyone have any comments, feedback or thoughts about this? >> > > > > > >> > > > > > Thanks, >> > > > > > >> > > > > > Tom >> > > > > > >> > > > > > On Tue, Mar 24, 2020 at 11:56 AM Tom Bentley < >> tbent...@redhat.com> >> > > > > wrote: >> > > > > > >> > > > > > > Hi, >> > > > > > > >> > > > > > > I've opened KIP-585 which is intended to provide a mechanism >> to >> > > > > > > conditionally apply SMTs in Kafka Connect. I'd be grateful >> for any >> > > > > > > feedback: >> > > > > > > >> > > > > >> > > > > >> > > > >> > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-585%3A+Conditional+SMT >> > > > > > > >> > > > > > > Many thanks, >> > > > > > > >> > > > > > > Tom >> > > > > > > >> > > > > >> > > > >> > > >> >>
