Yes, thank you everyone for your input! I will incorporate the latest round of test revisions and submit a VOTE thread later today :)
> On Feb 8, 2017, at 10:48 AM, Eno Thereska <eno.there...@gmail.com> wrote:
>
> 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
>>>>
>>>
>>>
>>>
>>
>
signature.asc
Description: Message signed with OpenPGP using GPGMail
