Steven,

Sounds like we can start a VOTE thread on this? Is the KIP up to date with all 
the latest comments?

Thanks
Eno
> On 8 Feb 2017, at 18:05, Matthias J. Sax <matth...@confluent.io> wrote:
> 
> I like this idea. But to get clean and concise PRs, I would prefer to
> have a JIRA and extra PR for this.
> 
> WDYT?
> 
> 
> -Matthias
> 
> On 2/8/17 9:35 AM, Guozhang Wang wrote:
>> 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
>>> 
>> 
>> 
>> 
> 

Reply via email to