KAFKA-6958: Overload KStream methods to allow to name operation name using the new Named class #6411

fhussonnois · 2019-03-08T19:16:48Z

This is the 3rd PR for the KIP-307.

NOTE : PR 6410 should be merge first

Thanks a lot for the review.

bbejeck

Overall this PR looks good to me, as any changes I'd suggest have already been addressed ion #6409 and #6410 this PR seems to be focused on adding the Named parameter overload to KStream and KStreamImpl and updating the tests.

bbejeck · 2019-03-12T20:10:59Z

@fhussonnois please rebase this PR

bbejeck · 2019-03-13T22:30:04Z

ping @vvcephei and @ableegoldman for reviews

bbejeck · 2019-04-18T22:47:34Z

Hi @fhussonnois #6410 was just merged, so if you could rebase this PR, we can make another pass and get this one merged as well.

Thanks for your work and patience!

fhussonnois · 2019-04-23T15:04:22Z

@bbejeck This PR has been rebase.

The KIP has been updated to include the newest methods that was missing :
flatTransformValues(ValueTransformerWithKeySupplier, Named, String... )
flatTransformValues(ValueTransformerSupplier, Named, String...)

bbejeck · 2019-04-23T16:27:08Z

Thanks @fhussonnois! taking a look at this today.

bbejeck

Thanks @fhussonnois just some very minor comments, otherwise LGTM

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImpl.java

bbejeck · 2019-04-24T23:33:29Z

call for second review any of @guozhangwang , @mjsax @vvcephei, @ableegoldman, @abbccdda

abbccdda · 2019-04-25T01:00:25Z

streams/src/test/java/org/apache/kafka/streams/StreamsBuilderTest.java

@@ -470,6 +479,233 @@ public void shouldUseSpecifiedNameForSinkProcessor() {
        assertSpecifiedNameForOperation(topology, "KSTREAM-SOURCE-0000000000", expected, "KSTREAM-SINK-0000000001");
    }

+    @Test
+    public void shouldUseSpecifiedNameForMapOperation() {


Do we consider refactoring these tests with one comment function? I could see L485-487 are repeated multiple times.

vvcephei

Thanks for the PR @fhussonnois !

According to my code analyser, several of the new methods are unused. Can we make sure we have test coverage verifying the new overloads work properly? I know this may seem paranoid, but we've had several embarrasing bugs in the Scala API, which should be similarly trivial, due to typos in the implementation code that could have been caught if they had any test coverage at all.

I had some thoughts about Named/NamedInternal, which I made inline.

There were several nits, which I just sent "suggestions" for.

Not sure if it helps, but I created a child page to your KIP, so the reviewers can strike though methods as you send PRs for them. (I just did the methods in this PR). I found it useful to go down the checklist for this PR.

Looks good, thanks!

streams/src/main/java/org/apache/kafka/streams/kstream/KStream.java

vvcephei · 2019-05-01T17:12:41Z

streams/src/test/java/org/apache/kafka/streams/StreamsBuilderTest.java

+    }
+
+    @Test
+    @SuppressWarnings("unchecked")


Suggested change

@SuppressWarnings("unchecked")

@SuppressWarnings({"unchecked", "rawtypes"})

streams/src/test/java/org/apache/kafka/streams/StreamsBuilderTest.java

streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImpl.java

fhussonnois · 2019-05-13T22:13:07Z

@bbejeck This PR has been rebased, most of the feedbacks has been resolved.

bbejeck · 2019-05-14T18:36:47Z

both Java 8 and Java 11 failed, test results already removed.

retest this please

fhussonnois · 2019-05-16T21:00:33Z

@bbejeck if everything is OK we this PR, could we merge it to move forward to the next one ?

bbejeck

Thanks @fhussonnois LGTM modulo comments left over from @vvcephei and @abbccdda

@fhussonnois

…ndex counter (#6754) When users provide a name for operation via the Streams DSL, we need to increment the counter used for auto-generated names to make sure any operators downstream of a named operator still produce a compatible name. This PR is a subset of #6411 by @fhussonnois. We need to merge this PR now because it covers cases when users name repartition topics or state stores. Updated tests to reflect the counter produces expected number even when the user provides a name. Matthias J. Sax <mjsax@apache.org>, John Roesler <john@confluent.io>

bbejeck · 2019-05-22T16:00:33Z

Hi @fhussonnois if you can rebase this PR we can then merge it, then move on to parts 4 and 5.

Thanks!

…using the new Named class Sub-task required to allow to define custom processor names with KStreams DSL(KIP-307) : - overload methods for stateless operations to accept a Named parameter (filter, filterNot, map, mapValues, foreach, peek, branch, transform, transformValue, flatTransform) - overload process method to accept a Named parameter - overload join/leftJoin/outerJoin methods

fhussonnois · 2019-05-27T20:06:41Z

Hi @bbejeck , this PR has been rebased.

bbejeck · 2019-05-30T14:39:16Z

both Java 8 and Java 11 failed, test results already cleared out.

retest this please

bbejeck · 2019-05-30T18:35:14Z

Java 8 passed Java 11 failed with kafka.integration.UncleanLeaderElectionTest.testUncleanLeaderElectionDisabled

retest this please

bbejeck · 2019-05-30T22:41:48Z

Java 8 failed, Java 11 passed

retest this please

bbejeck · 2019-05-31T02:03:29Z

retest this please

bbejeck · 2019-05-31T13:30:01Z

Merged #6411 into trunk.

bbejeck · 2019-05-31T13:30:16Z

Thank you @fhussonnois!

@fhussonnois

…ndex counter (apache#6754) When users provide a name for operation via the Streams DSL, we need to increment the counter used for auto-generated names to make sure any operators downstream of a named operator still produce a compatible name. This PR is a subset of apache#6411 by @fhussonnois. We need to merge this PR now because it covers cases when users name repartition topics or state stores. Updated tests to reflect the counter produces expected number even when the user provides a name. Matthias J. Sax <mjsax@apache.org>, John Roesler <john@confluent.io>

…using the new Named class (apache#6411) Sub-task required to allow to define custom processor names with KStreams DSL(KIP-307) : - overload methods for stateless operations to accept a Named parameter (filter, filterNot, map, mapValues, foreach, peek, branch, transform, transformValue, flatTransform) - overload process method to accept a Named parameter - overload join/leftJoin/outerJoin methods Reviewers: John Roesler <john@confluent.io>, Boyang Chen <boyang@confluent.io>, Bill Bejeck <bbejeck@gmail.com>

fhussonnois mentioned this pull request Mar 8, 2019

KAFKA-6958: Overload KTable methods to allow to name operation name using the new Named class #6412

Merged

fhussonnois changed the title ~~KAFKA-6958-SUB-TASK3~~ KAFKA-6958: Overload KStream methods to allow to name operation name using the new Named class Mar 8, 2019

fhussonnois force-pushed the KAFKA-6958-SUB-TASK3 branch from 68f4244 to 74a7a61 Compare March 8, 2019 19:56

bbejeck reviewed Mar 12, 2019

View reviewed changes

bbejeck requested review from guozhangwang and mjsax March 13, 2019 22:26

bbejeck added the streams label Mar 17, 2019

fhussonnois force-pushed the KAFKA-6958-SUB-TASK3 branch 6 times, most recently from 47e51ed to 0d9d8c2 Compare March 20, 2019 14:55

fhussonnois force-pushed the KAFKA-6958-SUB-TASK3 branch 3 times, most recently from 08a559e to dc354f4 Compare April 23, 2019 14:56

bbejeck reviewed Apr 24, 2019

View reviewed changes

abbccdda reviewed Apr 25, 2019

View reviewed changes

fhussonnois force-pushed the KAFKA-6958-SUB-TASK3 branch from dc354f4 to f7b5a09 Compare April 25, 2019 11:45

vvcephei requested changes May 1, 2019

View reviewed changes

bbejeck reviewed May 2, 2019

View reviewed changes

streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImpl.java Outdated Show resolved Hide resolved

streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImpl.java Outdated Show resolved Hide resolved

fhussonnois force-pushed the KAFKA-6958-SUB-TASK3 branch 3 times, most recently from 8ace90c to 796954a Compare May 3, 2019 21:22

fhussonnois force-pushed the KAFKA-6958-SUB-TASK3 branch from 796954a to 9aa0eed Compare May 13, 2019 21:53

bbejeck approved these changes May 17, 2019

View reviewed changes

bbejeck mentioned this pull request May 17, 2019

MINOR: Add select changes from 3rd KIP-307 PR for incrementing name index counter #6754

Merged

3 tasks

fhussonnois force-pushed the KAFKA-6958-SUB-TASK3 branch from 9aa0eed to 35e5b43 Compare May 22, 2019 20:40

bbejeck merged commit 78c55c8 into apache:trunk May 31, 2019

mjsax added the kip Requires or implements a KIP label Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-6958: Overload KStream methods to allow to name operation name using the new Named class #6411

KAFKA-6958: Overload KStream methods to allow to name operation name using the new Named class #6411

fhussonnois commented Mar 8, 2019

bbejeck left a comment

bbejeck commented Mar 12, 2019

bbejeck commented Mar 13, 2019

bbejeck commented Apr 18, 2019

fhussonnois commented Apr 23, 2019

bbejeck commented Apr 23, 2019

bbejeck left a comment

bbejeck commented Apr 24, 2019

abbccdda Apr 25, 2019

vvcephei left a comment

vvcephei May 1, 2019

fhussonnois commented May 13, 2019

bbejeck commented May 14, 2019

fhussonnois commented May 16, 2019

bbejeck left a comment

bbejeck commented May 22, 2019

fhussonnois commented May 27, 2019

bbejeck commented May 30, 2019

bbejeck commented May 30, 2019

bbejeck commented May 30, 2019

bbejeck commented May 31, 2019

bbejeck commented May 31, 2019

bbejeck commented May 31, 2019

	@SuppressWarnings("unchecked")
	@SuppressWarnings({"unchecked", "rawtypes"})

KAFKA-6958: Overload KStream methods to allow to name operation name using the new Named class #6411

KAFKA-6958: Overload KStream methods to allow to name operation name using the new Named class #6411

Conversation

fhussonnois commented Mar 8, 2019

bbejeck left a comment

Choose a reason for hiding this comment

bbejeck commented Mar 12, 2019

bbejeck commented Mar 13, 2019

bbejeck commented Apr 18, 2019

fhussonnois commented Apr 23, 2019

bbejeck commented Apr 23, 2019

bbejeck left a comment

Choose a reason for hiding this comment

bbejeck commented Apr 24, 2019

abbccdda Apr 25, 2019

Choose a reason for hiding this comment

vvcephei left a comment

Choose a reason for hiding this comment

vvcephei May 1, 2019

Choose a reason for hiding this comment

fhussonnois commented May 13, 2019

bbejeck commented May 14, 2019

fhussonnois commented May 16, 2019

bbejeck left a comment

Choose a reason for hiding this comment

bbejeck commented May 22, 2019

fhussonnois commented May 27, 2019

bbejeck commented May 30, 2019

bbejeck commented May 30, 2019

bbejeck commented May 30, 2019

bbejeck commented May 31, 2019

bbejeck commented May 31, 2019

bbejeck commented May 31, 2019