mjsax commented on code in PR #15740:
URL: https://github.com/apache/kafka/pull/15740#discussion_r1938342449
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImpl.java:
##########
@@ -796,7 +797,6 @@ public <KR> KGroupedStream<KR, V> groupBy(final
KeyValueMapper<? super K, ? supe
final GroupedInternal<KR, V> groupedInternal = new
GroupedInternal<>(grouped);
final ProcessorGraphNode<K, V> selectKeyMapNode =
internalSelectKey(keySelector, new NamedInternal(groupedInternal.name()));
- selectKeyMapNode.keyChangingOperation(true);
Review Comment:
I did have a closer look. I did mix up this flag, with `repartitionRequired`
parameter of `KStreamImpl` below, that we correctly pass in hardcode as `true`.
`keyChangingOperation` in contrast is for our "topology optimization /
rewriting" step, and it seems we are not using this flag for `groupBy(...)`
right now. However, we don't know if we might use it in the future, so I think
we should still keep it.
So technically, we have a little bit of a "split brain" problem, ie, two
code path that do related things independently. Ideally, we would unify both
code path to ensure they align. As this is independent of this PR, I would
propose to keep this line in this PR. I did file a ticket to track the idea of
refactoring, which is an independent piece of work:
https://issues.apache.org/jira/browse/KAFKA-18692
--
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
