Thanks for the explanation Jeyhun and Matthias.
I have just read through both KIP-149 and KIP-159 and am wondering if you
guys have considered a slight different approach for rich function, that is
to add the `RecordContext` into the apply functions as an additional
parameter. For example:
---------------------------
interface RichValueMapper<V, VR> {
VR apply(final V value, final RecordContext context);
}
...
// then in KStreams
<VR> KStream<K, VR> mapValues(ValueMapper<? super V, ? extends VR> mapper);
<VR> KStream<K, VR> mapValueswithContext(RichValueMapper <? super V, ?
extends VR> mapper);
-------------------------------
The caveat is that it will introduces more overloads; but I think the
#.overloads are mainly introduced by 1) serde overrides and 2)
state-store-supplier overides, both of which can be reduced in the near
future, and I felt this overloading is still worthwhile, as it has the
following benefits:
1) still allow lambda expressions.
2) clearer code path (do not need to "convert" from non-rich functions to
rich functions)
Maybe this approach has already been discussed and I may have overlooked in
the email thread; anyways, lmk.
Guozhang
On Thu, Jun 1, 2017 at 10:18 PM, Matthias J. Sax <matth...@confluent.io>
wrote:
> I agree with Jeyhun. As already mention, the overall API improvement
> ideas are overlapping and/or contradicting each other. For this reason,
> not all ideas can be accomplished and some Jira might just be closed as
> "won't fix".
>
> For this reason, we try to do those KIP discussion with are large scope
> to get an overall picture to converge to an overall consisted API.
>
>
> @Jeyhun: about the overloads. Yes, we might get more overload. It might
> be sufficient though, to do a single xxxWithContext() overload that will
> provide key+value+context. Otherwise, if might get too messy having
> ValueMapper, ValueMapperWithKey, ValueMapperWithContext,
> ValueMapperWithKeyWithContext.
>
> On the other hand, we also have the "builder pattern" idea as an API
> change and this might mitigate the overload problem. Not for simple
> function like map/flatMap etc but for joins and aggregations.
>
>
> On the other hand, as I mentioned in an older email, I am personally
> fine to break the pure functional interface, and add
>
> - interface WithRecordContext with method `open(RecordContext)` (or
> `init(...)`, or any better name) -- but not `close()`)
>
> - interface ValueMapperWithRecordContext extends ValueMapper,
> WithRecordContext
>
> This would allow us to avoid any overload. Of course, we don't get a
> "pure function" interface and also sacrifices Lambdas.
>
>
>
> I am personally a little bit undecided what the better option might be.
> Curious to hear what other think about this trade off.
>
>
> -Matthias
>
>
> On 6/1/17 6:13 PM, Jeyhun Karimov wrote:
> > Hi Guozhang,
> >
> > It subsumes partially. Initially the idea was to support RichFunctions
> as a
> > separate interface. Throughout the discussion, however, we considered
> maybe
> > overloading the related methods (with RecodContext param) is better
> > approach than providing a separate RichFunction interface.
> >
> > Cheers,
> > Jeyhun
> >
> > On Fri, Jun 2, 2017 at 2:27 AM Guozhang Wang <wangg...@gmail.com> wrote:
> >
> >> Does this KIP subsume this ticket as well?
> >> https://issues.apache.org/jira/browse/KAFKA-4125
> >>
> >> On Sat, May 20, 2017 at 9:05 AM, Jeyhun Karimov <je.kari...@gmail.com>
> >> wrote:
> >>
> >>> Dear community,
> >>>
> >>> As we discussed in KIP-149 [DISCUSS] thread [1], I would like to
> initiate
> >>> KIP for rich functions (interfaces) [2].
> >>> I would like to get your comments.
> >>>
> >>>
> >>> [1]
> >>> http://search-hadoop.com/m/Kafka/uyzND1PMjdk2CslH12?subj=
> >>> Re+DISCUSS+KIP+149+Enabling+key+access+in+
> ValueTransformer+ValueMapper+
> >>> and+ValueJoiner
> >>> [2]
> >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>> 159%3A+Introducing+Rich+functions+to+Streams
> >>>
> >>>
> >>> Cheers,
> >>> Jeyhun
> >>> --
> >>> -Cheers
> >>>
> >>> Jeyhun
> >>>
> >>
> >>
> >>
> >> --
> >> -- Guozhang
> >>
>
>
--
-- Guozhang
