KAFKA-6958: Overload methods for group and windowed stream to allow to name operation name using the new Named class by fhussonnois · Pull Request #6413 · apache/kafka

fhussonnois · 2019-03-08T19:24:05Z

This is the last PR for the KIP-307.

NOTE : PR 6412 should be merge first

Thanks a lot for the review.

bbejeck

Made a pass over SessionedXX TimeWindowed and aggregate builders. Just one minor comment otherwise looks reasonable.

bbejeck · 2019-03-13T22:27:52Z

@fhussonnois please rebase this PR

bbejeck · 2019-03-13T22:28:08Z

ping @vvcephei @ableegoldman for reviews

bbejeck · 2019-06-17T22:06:13Z

@fhussonnois #6412 is merged now, can you rebase this PR and ping us when it's ready for review?

fhussonnois · 2019-07-01T19:58:11Z

@bbejeck This PR has been rebase (sorry I just remember I've forgotten to ping you) thank you

bbejeck · 2019-07-17T15:33:21Z

This PR has been rebase (sorry I just remember I've forgotten to ping you) thank you

@fhussonnois no worries! Thanks for the rebase, I'll make a pass on this last PR soon

bbejeck · 2019-07-17T15:33:58Z

retest this please

bbejeck · 2019-07-17T16:31:41Z

@fhussonnois the build failures are related, the changes need to go into the scala API as well - Execution failed for task ':streams:streams-scala:compileTestScala'

bbejeck · 2019-07-19T19:25:11Z

Java 8 and Java 11 Scala 2.12 failed with

      fatal: unable to connect to github.com:
      github.com: Temporary failure in name resolution

Java 11 2.13 failed with SaslScramSslEndToEndAuthorizationTest.testNoDescribeProduceOrConsumeWithoutTopicDescribeAcl

retest this please

bbejeck · 2019-07-24T22:18:56Z

Test results already removed.

retest this please

bbejeck

@fhussonnois thanks for your persistence on KIP-307! I've made a pass over this final PR and overall it looks good, I only have minor comments.

bbejeck · 2019-07-29T15:23:45Z

shouldn't the table-source suffix go with the sourceName?

@bbejeck, Yes, that seems more logical. I made the modification for the global table too. Also, I think the suffix "-source" is sufficient.

bbejeck · 2019-07-29T15:32:02Z

should this have a suffix at all? I don't think the funcName requires one.

If it does, at a minimum we should not use -sink as it's used above and doesn't accurately describe its role.

@bbejeck ,You're right, it seems OK to remove the suffix from the function name. The user should ensure that a unique name is given to each node. So, I've removed the suffix and I also add a unit test.

bbejeck · 2019-07-29T15:55:11Z

Java 11, Scala 2.12 failed, Java 8 and Java 11 Scala 2.11 passed, test results already cleaned up.

retest this please

bbejeck

Thanks for the update @fhussonnois LGTM.

bbejeck · 2019-08-20T15:58:57Z

Java 8/2.11 and Java 11/2.13 failed
Java 11/2.12 passed
test results already cleaned out

retest this please

bbejeck · 2019-08-20T15:59:49Z

ping @guozhangwang, @mjsax, @vvcephei, @ableegoldman, @cadonna, @abbccdda
for a second review

vvcephei

Hey @fhussonnois , thanks for this latest edition in the ongoing saga of adding Named :)

This pretty much looks good to me, I just had a few comments and questions.

The big concern is the Scala DSL. It seems like this is actually a significant problem.

Thanks,
-John

vvcephei · 2019-08-22T15:43:26Z

Would this result in a different name for the source than the prior code? (Not sure if it matters...)

Hey @fhussonnois or @bbejeck , what do you think about this?

vvcephei · 2019-08-22T15:45:35Z

did you want to add a null check for this as well (like you did for aggregate)?

vvcephei · 2019-08-22T15:57:11Z

Were these changes necessary to disambiguate the methods? It would be important to know, because it would constitute an API-breaking change in the Scala DSL.

Ok, I've just confirmed that this is a breaking change in the Scala DSL. I'm not sure why but introducing the KGroupedStream#aggregate(Initializer, Aggregator, Named) method creates ambiguity about how the type system should infer the generic parameters to Materialized here.

This also resolves the ambiguity:

val stream2: KStreamJ[String, Integer] = mappedStream.groupByKey .aggregate(initializer, aggregator, MaterializedJ.`with`(Serdes.String, SerdesJ.Integer): MaterializedJ[String, Integer, KeyValueStore[Bytes, Array[Byte]]]) .toStream

Either way, it's a breaking change, which we should probably not merge.

One simple fix is simply not to add that particular overload. The full expression with Named and Materialized would still be present.

vvcephei · 2019-08-22T16:08:13Z

It seems weird that these changed. Is that expected and ok?

Hey @fhussonnois or @bbejeck , what do you think about this?

bbejeck · 2019-09-04T21:17:36Z

@fhussonnois regarding the overloads
KGroupedStream#aggregate(initializer, aggregator, named) and KGroupedStream#reduce(reducer, named) let's go ahead and just drop those for now and we can figure out how to work them in later on.

The KIP will need to get updated with this information as well.

…o name operation name using the new Named class Sub-task required to allow to define custom processor names with KStreams DSL(KIP-307) : - overload method for KGroupedStream to accept a Named parameter - overload method for KGroupedTable to accept a Named parameter - overload method for TimeWindowedKStream to accept a Named parameter - overload method for SessionWindowedKStream to accept a Named parameter

bbejeck · 2019-09-10T19:43:49Z

Java 8/2.11, and Java 11/2.13 passed
Java 11/2.12 failed Build timed out (after 240 minutes). Marking the build as aborted.

retest this please

bbejeck · 2019-09-20T18:19:55Z

Java 11/2.13 failed, test results already gone

retest this please

vvcephei

This LGTM now. I had one unanswered question, but I don't think we need to block on it. We can always do a follow-up patch if it's important.

Thanks so much @fhussonnois , for all your work on this!

vvcephei · 2019-09-20T19:03:44Z

Hey @fhussonnois or @bbejeck , what do you think about this?

vvcephei · 2019-09-20T19:04:03Z

Hey @fhussonnois or @bbejeck , what do you think about this?

bbejeck · 2019-09-23T17:12:19Z

Merged #6413 into trunk.

bbejeck · 2019-09-23T17:12:35Z

Thanks for all the hard work @fhussonnois !!

fhussonnois · 2019-09-23T18:28:22Z

Thank you very much @bbejeck for you help on that KIP.

mjsax · 2019-09-24T16:24:48Z

Congrats @fhussonnois! Great work!

Conflicts: * .gitignore: addition of clients/src/generated-test was near local additions for support-metrics. * checkstyle/suppressions.xml: upstream refactoring of exclusions for generator were near the local changes for support-metrics. * gradle.properties: scala version bump caused a minor conflict due to the kafka version change locally. gradle/dependencies.gradle: bcpkix version bump was near avro additions in the local version. * apache-github/trunk: (49 commits) KAFKA-8471: Replace control requests/responses with automated protocol (apache#7353) MINOR: Don't generate unnecessary strings for debug logging in FetchSessionHandler (apache#7394) MINOR:fixed typo and removed outdated varilable name (apache#7402) KAFKA-8934: Create version file during build for Streams (apache#7397) KAFKA-8319: Make KafkaStreamsTest a non-integration test class (apache#7382) KAFKA-6883: Add toUpperCase support to sasl.kerberos.principal.to.local rule (KIP-309) KAFKA-8907; Return topic configs in CreateTopics response (KIP-525) (apache#7380) MINOR: Address review comments for KIP-504 authorizer changes (apache#7379) MINOR: add versioning to request and response headers (apache#7372) KAFKA-7273: Extend Connect Converter to support headers (apache#6362) MINOR: improve the Kafka RPC code generator (apache#7340) MINOR: Improve the org.apache.kafka.common.protocol code (apache#7344) KAFKA-8880: Docs on upgrade-guide (apache#7385) KAFKA-8179: do not suspend standby tasks during rebalance (apache#7321) KAFKA-8580: Compute RocksDB metrics (apache#7263) KAFKA-8880: Add overloaded function of Consumer.committed (apache#7304) HOTFIX: fix Kafka Streams upgrade note for broker backward compatibility (apache#7363) KAFKA-8848; Update system tests to use new AclAuthorizer (apache#7374) MINOR: remove unnecessary null check (apache#7299) KAFKA-6958: Overload methods for group and windowed stream to allow to name operation name using the new Named class (apache#6413) ...

fhussonnois force-pushed the KAFKA-6958-SUB-TASK5 branch from c1e25ea to 7d40dab Compare March 8, 2019 19:58

bbejeck added the streams label Mar 8, 2019

bbejeck reviewed Mar 13, 2019

View reviewed changes

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KGroupedTableImpl.java Outdated

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

fhussonnois force-pushed the KAFKA-6958-SUB-TASK5 branch 5 times, most recently from 67b0a21 to 0adbcf5 Compare March 20, 2019 14:54

fhussonnois force-pushed the KAFKA-6958-SUB-TASK5 branch from 0adbcf5 to 012b883 Compare June 18, 2019 13:27

fhussonnois force-pushed the KAFKA-6958-SUB-TASK5 branch 2 times, most recently from dee0868 to 211f656 Compare July 18, 2019 10:14

bbejeck reviewed Jul 29, 2019

View reviewed changes

fhussonnois force-pushed the KAFKA-6958-SUB-TASK5 branch 3 times, most recently from 0db594f to 78e1950 Compare August 8, 2019 09:52

bbejeck approved these changes Aug 20, 2019

View reviewed changes

vvcephei requested changes Aug 22, 2019

View reviewed changes

fhussonnois force-pushed the KAFKA-6958-SUB-TASK5 branch from 78e1950 to ccdf00c Compare September 9, 2019 20:35

vvcephei approved these changes Sep 20, 2019

View reviewed changes

bbejeck mentioned this pull request Sep 23, 2019

fix: fix test for KIP-307 final PR confluentinc/ksql#3402

Merged

2 tasks

bbejeck merged commit beac4c7 into apache:trunk Sep 23, 2019

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

Conversation

fhussonnois commented Mar 8, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bbejeck commented Mar 13, 2019

Uh oh!

bbejeck commented Mar 13, 2019

Uh oh!

bbejeck commented Jun 17, 2019

Uh oh!

fhussonnois commented Jul 1, 2019

Uh oh!

bbejeck commented Jul 17, 2019

Uh oh!

bbejeck commented Jul 17, 2019

Uh oh!

bbejeck commented Jul 17, 2019

Uh oh!

bbejeck commented Jul 19, 2019

Uh oh!

bbejeck commented Jul 24, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Jul 29, 2019

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Aug 20, 2019

Uh oh!

bbejeck commented Aug 20, 2019

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbejeck commented Sep 10, 2019

Uh oh!

bbejeck commented Sep 20, 2019

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

bbejeck commented Sep 4, 2019 •

edited

Loading