The KIP proposal LGTM, thanks Steven!
One meta comment on the PR itself: I'm wondering if we could refactoring
the implementation of `KStream.print() / writeAsText()` to just be a
special impl of `peek()` then, like we did for `count` as for `aggregate`?
I.e. we can replace the `KeyValuePrinter` class with an internal ForEach
impl within `peek()`.
Guozhang
On Tue, Feb 7, 2017 at 11:09 AM, Gwen Shapira <g...@confluent.io> wrote:
> Far better! Thank you!
>
> On Tue, Feb 7, 2017 at 10:19 AM, Steven Schlansker
> <sschlans...@opentable.com> wrote:
> > Thanks for the feedback. I improved the javadoc a bit, do you like it
> better?
> >
> > /**
> > * Perform an action on each record of {@code KStream}.
> > * This is a stateless record-by-record operation (cf. {@link
> #process(ProcessorSupplier, String...)}).
> > *
> > * Peek is a non-terminal operation that triggers a side effect
> (such as logging or statistics collection)
> > * and returns an unchanged stream.
> > *
> > * Note that since this operation is stateless, it may execute
> multiple times for a single record in failure cases.
> > *
> > * @param action an action to perform on each record
> > * @see #process(ProcessorSupplier, String...)
> > */
> > KStream<K, V> peek(final ForeachAction<? super K, ? super V> action);
> >
> > Updated in-place on the PR.
> >
> >> On Feb 7, 2017, at 2:19 AM, Michael Noll <mich...@confluent.io> wrote:
> >>
> >> Many thanks for the KIP and the PR, Steven!
> >>
> >> My opinion, too, is that we should consider including this.
> >>
> >> One thing that I would like to see clarified is the difference between
> the
> >> proposed peek() and existing functions map() and foreach(), for
> instance.
> >> My understanding (see also the Java 8 links below) is that:
> >>
> >> - Like `map`, `peek` will return a KStream. This also means that,
> unlike
> >> `foreach`, `peek` is not a terminal operation.
> >> - The main purpose of `peek` is, similar to `foreach`, the *side
> effects*
> >> (such as the metrics counter example in the KIP) -- and, on a related
> note,
> >> also to express your *intent* to achieve such side effects in the first
> >> place (which is similar to when to use `foreach` rather than `map`); and
> >> typically you should not (must not?) modify the underlying stream itself
> >> (unlike `map`, which is supposed to do exactly that).
> >>
> >> For reference, here are the descriptions of peek, map, foreach in Java
> 8.
> >> I could have also included links to StackOverflow questions where people
> >> were confused about when (not) to use peek. ;-)
> >>
> >> https://docs.oracle.com/javase/8/docs/api/java/util/
> stream/Stream.html#peek-java.util.function.Consumer-
> >> https://docs.oracle.com/javase/8/docs/api/java/util/
> stream/Stream.html#map-java.util.function.Function-
> >> https://docs.oracle.com/javase/8/docs/api/java/util/
> stream/Stream.html#forEach-java.util.function.Consumer-
> >>
> >> Best wishes,
> >> Michael
> >>
> >>
> >>
> >>
> >>
> >>
> >> On Tue, Feb 7, 2017 at 10:37 AM, Damian Guy <damian....@gmail.com>
> wrote:
> >>
> >>> Hi Steven,
> >>> Thanks for the KIP. I think this is a worthy addition to the API.
> >>>
> >>> Thanks,
> >>> Damian
> >>>
> >>> On Tue, 7 Feb 2017 at 09:30 Eno Thereska <eno.there...@gmail.com>
> wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> I like the proposal, thank you. I have found it frustrating myself
> not to
> >>>> be able to understand simple things, like how many records have been
> >>>> currently processed. The peek method would allow those kinds of
> >>> diagnostics
> >>>> and debugging.
> >>>>
> >>>> Gwen, it is possible to do this with the existing functionality like
> map,
> >>>> but you'd have to fake the map method. Also, it is not great using map
> >>> for
> >>>> things it was not intended for. Having an explicit peek makes it
> clearer
> >>> in
> >>>> my opinion.
> >>>>
> >>>> Thanks
> >>>> Eno
> >>>>
> >>>>> On 7 Feb 2017, at 03:20, Gwen Shapira <g...@confluent.io> wrote:
> >>>>>
> >>>>> I've read the wiki and am unclear about the proposal. Can you provide
> >>>>> something like a Javadoc for peek()? What would this method do?
> >>>>>
> >>>>> Also, forgive me if I'm missing an important point here, but can't I
> >>>>> put the println statement in a map()?
> >>>>>
> >>>>> On Mon, Feb 6, 2017 at 5:48 PM, Matthias J. Sax <
> matth...@confluent.io
> >>>>
> >>>> wrote:
> >>>>>> Steven,
> >>>>>>
> >>>>>> Thanks for your KIP. I move this discussion to dev mailing list --
> >>> KIPs
> >>>>>> need to be discussed there (and can be cc'ed to user list).
> >>>>>>
> >>>>>> Can you also add the KIP to the table "KIPs under discussion":
> >>>>>>
> >>>>>>
> >>>> https://cwiki.apache.org/confluence/display/KAFKA/
> >>> Kafka+Improvement+Proposals#KafkaImprovementProposals-
> KIPsunderdiscussion
> >>>>>>
> >>>>>>
> >>>>>> Thanks.
> >>>>>>
> >>>>>>
> >>>>>> -Matthias
> >>>>>>
> >>>>>> On 2/6/17 3:35 PM, Steven Schlansker wrote:
> >>>>>>> Hello users@kafka,
> >>>>>>>
> >>>>>>> I would like to propose a small KIP on the Streams framework
> >>>>>>> that simply adds a KStream#peek implementation.
> >>>>>>>
> >>>>>>>
> >>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >>> 121%3A+Add+KStream+peek+method
> >>>>>>> https://issues.apache.org/jira/browse/KAFKA-4720
> >>>>>>> https://github.com/apache/kafka/pull/2493
> >>>>>>>
> >>>>>>> Please consider my contribution and hopefully you all like it and
> >>>> agree that it should be merged into 0.10.3 :)
> >>>>>>> If not, be gentle, this is my first KIP!
> >>>>>>>
> >>>>>>> Happy Monday,
> >>>>>>> Steven
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> Gwen Shapira
> >>>>> Product Manager | Confluent
> >>>>> 650.450.2760 <(650)%20450-2760> | @gwenshap
> >>>>> Follow us: Twitter | blog
> >>>>
> >>>>
> >>>
> >
>
>
>
> --
> Gwen Shapira
> Product Manager | Confluent
> 650.450.2760 | @gwenshap
> Follow us: Twitter | blog
>
--
-- Guozhang
