mjsax commented on PR #18314: URL: https://github.com/apache/kafka/pull/18314#issuecomment-2568465445
Did not look into HTML yet... ``` The migration process consists of: Replace Transformer with Processor or ValueTransformer with FixedKeyProcessor; Replace record key and value with Record or FixedKeyRecord; Rewrite the transform method of Transformer and ValueTransformer as process or processValues; Use the new Record or FixedKeyRecord as argument of the renamed method; Rewrite the return type of the renamed method to void and forward the record through the context; and finally Change the KStream call of the transform method to process or processValues. ``` This seems to be _very_ detailed? When saying `Replace Transformer with Processor or ValueTransformer with FixedKeyProcessor`, it seems to be more or less clear that one also needs to do `Replace record key and value with Record or FixedKeyRecord;` and `Rewrite the transform method of Transformer and ValueTransformer as process or processValues;`? This one (`Use the new Record or FixedKeyRecord as argument of the renamed method;`) seems to be redundant anyway? I am worries that people get intimidated, as it sound more complex than it actually is? Might it be better to keep it simpler? Btw: You did not work on the code change for this, but we also did remove `KStream#process(/*old Processor*/)` which is also replaced by the same `KStream#process(/*new api.Processor*/`). It would be great to add this to this migration guide, too. Example 1: `logStream.flatTransform(() -> new LogSeverityTransformer())` vs `logStream.process(LogSeverityProcessor::new)` -- why are you switching the pattern from lambda to method reference? This can be confusing for reads. Stick with either one. Seem you are using method reference in the other examples. Also, the `to(...)` operator is missing in the `process()` example. Example 2: `SlangReplacementTransformer implements ValueTransformer<String, Iterable<String>>` -- it seem the result type should be `String` not `Iterable<String>` and the `returns` statement should concatenate the words (plus adding back blanks) similar to the `Processor` example? Example 3: ``` // Send a discount notification context.forward(new Record<>(record.key(), DISCOUNT_NOTIFICATION_MESSAGE, record.timestamp())); ``` Why no `context.forward(record.withValue(DISCOUNT_NOTIFICATION_MESSAGE)` ? Should we also mention `ValueTransformerWithKey` that was removed? I noticed that the `ValueTransformer` example now needs to add the key-type (`Void` in your example), which was not necessary for `ValueTransformer` as it does not have access to the key anyway, however, `ValueTransformerWithKey` has access to it, and thus three generic types, so basically the new `FixedKeyProcessor` is closer to `ValueTransformerWithKey` compared to `ValueTransformer`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
