mjsax commented on code in PR #15482:
URL: https://github.com/apache/kafka/pull/15482#discussion_r1728203037
##########
streams/src/main/java/org/apache/kafka/streams/kstream/internals/KTableImpl.java:
##########
@@ -1229,7 +1229,7 @@ private <VR, KO, VO> KTable<K, VR>
doJoinOnForeignKey(final KTable<KO, VO> forei
final StreamPartitioner<K, SubscriptionResponseWrapper<VO>>
foreignResponseSinkPartitioner =
tableJoinedInternal.partitioner() == null
- ? (topic, key, subscriptionResponseWrapper,
numPartitions) -> subscriptionResponseWrapper.getPrimaryPartition()
+ ? (topic, key, subscriptionResponseWrapper,
numPartitions) ->
Optional.of(Collections.singleton(subscriptionResponseWrapper.getPrimaryPartition()))
Review Comment:
I don't think it's worth to add an overloaded constructor. This adds more
noise than it simplifies code IMHO.
However, looking into the code mode closely, it seems that
`getPrimaryPartition ` is called elsewhere, so changing the return type to
`Optional` might actually not be ideal?
However, we need to take into account that `getPrimaryPartition` could
return `null` for which we should not return a singleton collection with `null`
in it here, but rather an `Optional.empty()` (please correct me if I am wrong).
##########
streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java:
##########
@@ -1569,7 +1569,7 @@ private void assertNextOutputRecord(final
TestRecord<String, String> record,
}
private StreamPartitioner<Object, Object> constantPartitioner(final
Integer partition) {
Review Comment:
@caioguedes Any thoughts?
--
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]
