big-andy-coates commented on a change in pull request #9156:
URL: https://github.com/apache/kafka/pull/9156#discussion_r487057186
##########
File path:
streams/src/main/java/org/apache/kafka/streams/kstream/internals/KTableAggregate.java
##########
@@ -47,8 +47,10 @@
}
@Override
- public void enableSendingOldValues() {
+ public boolean enableSendingOldValues(final boolean onlyIfMaterialized) {
Review comment:
I did initially go with this. However, it seems really unintuitive.
Given a param such as `forceMaterialization` then it's clear that
`enableSendingOldValues(true)` will force materialization, but what does
`enabledSendingOldValues(false)` mean? OK, we're not forcing materialization,
but actually the method won't enable sending old values if the param is `false`
and its not already materialized.
Even if we go with `maybeEnableSendingOldValues(enforceMaterialization)` I
still would argue the semantics are not intuitive from the names alone.
(Though of course JavaDocs would help).
However, given `enableSendingOldValues(onlyIfMaterialized)` is, IMHO, very
intuitive. `enableSendingOldValues(true)` will only enable sending old values
if already materialized, whereas `enableSendingOldValues(false)` will always
enable sending old values, materializing as necessary, must like the previous
`enableSendingOldValues()` method did.
Likewise, if we stick with `enableSendingOldValues(onlyIfMaterialized)` then
I don't think we need to include a `maybe` or `IfPossible` in the name as this
is implied already by the `onlyIf`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
