eduwercamacaro commented on code in PR #20403:
URL: https://github.com/apache/kafka/pull/20403#discussion_r2363588877
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java:
##########
@@ -1104,11 +1104,18 @@ private void initializeTopology() {
// initialize the task by initializing all its processor nodes in the
topology
log.trace("Initializing processor nodes of the topology");
for (final ProcessorNode<?, ?, ?, ?> node : topology.processors()) {
- processorContext.setCurrentNode(node);
+ final ProcessorRecordContext recordContext = new
ProcessorRecordContext(
+ time.milliseconds(),
Review Comment:
Yes, `ProcessorRecordContext` is an internal class, and it shouldn’t be used
by the users. However, I do think that the subclass adds an extra level of
semantics to the code. For contributors, it could be confusing to see a
`ProcessorRecordContext` instance with no topic and partition information.
That’s why I was thinking of the subclass that is more specific and tells
exactly why we need to pass these values to the parent class.
I don’t see a risk of missing any code update with this approach, as it is
being used in only one place: during topology initialization. However, I do see
this risk of passing the wrong value when using the approach of passing a
function that returns the timestamp. That’s another reason why I prefer the
subclass approach.
As you said, it works either way. It is mostly a personal opinion. I’m also
fine either way.
I have updated the code to use the subclass, so we can continue the
discussion, but we can definitely change this based on your feedback.
--
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]
