Skip to content

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

Merged
bbejeck merged 1 commit intoapache:trunkfrom
fhussonnois:KAFKA-6958-SUB-TASK4
Jun 17, 2019
Merged

KAFKA-6958: Overload KTable methods to allow to name operation name using the new Named class#6412
bbejeck merged 1 commit intoapache:trunkfrom
fhussonnois:KAFKA-6958-SUB-TASK4

Conversation

@fhussonnois
Copy link
Copy Markdown
Contributor

@fhussonnois fhussonnois commented Mar 8, 2019

Hi @mjsax @bbejeck

This is the 4th PR for the KIP-307.
NOTE :PR 6411 should be merged first

Thanks a lot for the review.

@fhussonnois fhussonnois mentioned this pull request Mar 8, 2019
Merged
@fhussonnois fhussonnois changed the title KAFKA-6958-SUB-TASK4 KAFKA-6958: Overload KTable methods to allow to name operation name using the new Named class Mar 8, 2019
@fhussonnois fhussonnois force-pushed the KAFKA-6958-SUB-TASK4 branch from 6405ffa to 904ee34 Compare March 8, 2019 19:58
bbejeck
bbejeck reviewed Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a pass over the KTable and KTableImpl. Overall looks good but IMHO I think we should add KTable.toStream(). Also, there aren't tests for KTable.

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/KTable.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't specify this in the KIP but I think toStream() and toStream(mapper) should also have overrides withNamed

Copy link
Copy Markdown
Contributor Author

@fhussonnois fhussonnois Mar 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the KIP and the PR to the two methods above

Copy link
Copy Markdown
Contributor

@vvcephei vvcephei Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@bbejeck bbejeck requested review from guozhangwang and mjsax March 13, 2019 22:26
@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Mar 13, 2019

ping @vvcephei and @ableegoldman for reviews

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Mar 13, 2019

@fhussonnois please rebase this PR

@fhussonnois fhussonnois force-pushed the KAFKA-6958-SUB-TASK4 branch 5 times, most recently from 6a37f11 to bb40cf2 Compare March 20, 2019 14:55
@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented May 31, 2019

@fhussonnois with #6411 merged can you rebase this PR? With the bulk of the work done so far, I think we can get this PR and the last PR for KIP-307 merged relatively quickly.

@fhussonnois fhussonnois force-pushed the KAFKA-6958-SUB-TASK4 branch 2 times, most recently from f0bf5e0 to 3d69114 Compare May 31, 2019 21:34
@fhussonnois
Copy link
Copy Markdown
Contributor Author

fhussonnois commented May 31, 2019

Hi @bbejeck, this PR is rebased and ready for review. Thanks

@bellemare bellemare mentioned this pull request Jun 4, 2019
Merged
bbejeck
bbejeck reviewed Jun 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fhussonnois! I just have minor comments, but otherwise, this looks good.

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/KTable.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: ditto

I'll go ahead and say we should do so for all examples (when possible of course) in the Javadoc and I won't point out anymore from this point.

Copy link
Copy Markdown
Contributor Author

@fhussonnois fhussonnois Jun 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the javadoc has been updated - there was also an issue on the method name (ie : mapValue to mapValues)

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/KTable.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: change mapValue to take a lambda vs. an anonymous ValueMapper

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/KTable.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same here

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/KTable.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why these changes?

Copy link
Copy Markdown
Contributor Author

@fhussonnois fhussonnois Jun 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, these changes come from javadoc shift due to new methods added.

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/KTable.java Outdated
Copy link
Copy Markdown
Member

@bbejeck bbejeck Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Copy Markdown
Contributor Author

@fhussonnois fhussonnois Jun 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, these changes come from javadoc shift due to new methods added.

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KTableImpl.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KTableImpl.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/NamedInternal.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/SuppressTopologyTest.java Outdated
vvcephei
vvcephei requested changes Jun 6, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @fhussonnois , thanks for this!

Once you address Bill's comments, this will be fine by me.

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/KTable.java Outdated
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei Jun 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KTableImpl.java Outdated
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/SuppressTopologyTest.java Outdated
@fhussonnois
KAFKA-6958: Overload KTable methods to allow to name operation name u…
f254c4f 
…sing the new Named class

Sub-task required to allow to define custom processor names with KStreams DSL(KIP-307) :
@fhussonnois fhussonnois force-pushed the KAFKA-6958-SUB-TASK4 branch from 3d69114 to f254c4f Compare June 15, 2019 10:02
vvcephei
vvcephei approved these changes Jun 17, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

bbejeck
bbejeck approved these changes Jun 17, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fhussonnois, LGTM.

@bbejeck bbejeck merged commit 6d6366c into apache:trunk Jun 17, 2019
@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Jun 17, 2019

Merged #6412 into trunk.

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Jun 17, 2019

Thanks again @fhussonnois!

@mjsax mjsax added the kip Requires or implements a KIP label Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbejeck bbejeck bbejeck approved these changes

@vvcephei vvcephei vvcephei approved these changes

@guozhangwang guozhangwang Awaiting requested review from guozhangwang

@mjsax mjsax Awaiting requested review from mjsax

Assignees

No one assigned

Labels

kip Requires or implements a KIP streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fhussonnois @bbejeck @vvcephei @mjsax