jeqo commented on a change in pull request #11481:
URL: https://github.com/apache/kafka/pull/11481#discussion_r754687365
##########
File path:
streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamSessionWindowAggregateProcessorTest.java
##########
@@ -116,6 +116,7 @@ private void setup(final boolean enableCache) {
results.add(new KeyValueTimestamp<>(record.key(),
record.value(), record.timestamp()));
}
};
+ context.setTime(0L);
Review comment:
Agree.
Here's another interesting finding:
https://github.com/apache/kafka/blob/4fd71a7ef1c79ec3f110ee2f9d38fa7d5194937e/streams/src/main/java/org/apache/kafka/streams/processor/internals/AbstractProcessorContext.java#L174-L185
AbstractProcessorContext is returning `0` when record context is null—as
default value I assume. Wondering if we should align the defaults to zero, or
`-1`.
> If you wanted to do a "proper" fix, you could insert a method like
invokeProcessor(Record), which would first set the timestamp on the context and
then invoke the Processor.
I'm failing to see how this `invokeProcessor` should be defined. If as part
of `KStreamSessionWindowAggregateProcessorTest`, I understand `setup` is
already handling this initialiazation before processing—though I might be
misunderstanding the proposed fixed.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
