Skip to content

KAFKA-9011: Scala bindings for flatTransform and flatTransformValues in KStream - #7520

Merged
bbejeck merged 2 commits into
apache:trunkfrom
kokachev:flatTransformScala
Nov 12, 2019
Merged

KAFKA-9011: Scala bindings for flatTransform and flatTransformValues in KStream#7520
bbejeck merged 2 commits into
apache:trunkfrom
kokachev:flatTransformScala

Conversation

@kokachev

@kokachev kokachev commented Oct 15, 2019

Copy link
Copy Markdown
Contributor

@cadonna

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@mjsax mjsax added the streams label Oct 15, 2019
@mjsax

mjsax commented Oct 15, 2019

Copy link
Copy Markdown
Member

Call for review @bbejeck @vvcephei

@kokachev
kokachev force-pushed the flatTransformScala branch 2 times, most recently from c5b3cb5 to e133fa8 Compare October 15, 2019 10:48
@vvcephei

vvcephei commented Oct 18, 2019

Copy link
Copy Markdown
Contributor

Hey @kokachev ,

Thanks for the contribution! This looks good to me, but can you add some tests exercising the new methods? We have had quite a few issues in the past with Scala implicits behaving unintuitively, so now it's hard to trust even code that looks "obviously correct".

Thanks,
-John

bbejeck
bbejeck reviewed Oct 18, 2019
View reviewed changes

@bbejeck bbejeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @kokachev over LGTM, but I agree with @vvcephei that we'll need some tests using the new methods.

@kokachev

kokachev commented Oct 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@bbejeck , @vvcephei, Added tests for new functions as requested. Got a bit excited and added test for transform, which wasn't in a scope ;)

@kokachev

kokachev commented Oct 29, 2019

Copy link
Copy Markdown
Contributor Author

retest this please

@kokachev

kokachev commented Oct 31, 2019

Copy link
Copy Markdown
Contributor Author

@bbejeck , @vvcephei , @mjsax Can you guys please merge it to 2.3.2 or 2.4? JDK8 build is failing due to test broken upstream.

@kokachev

kokachev commented Nov 8, 2019

Copy link
Copy Markdown
Contributor Author

retest this please

vvcephei
vvcephei approved these changes Nov 11, 2019
View reviewed changes

@vvcephei vvcephei left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kokachev ,

Sorry for the delay. These tests look great to me. I think we can go ahead and merge this PR!

Thanks,
-John

@vvcephei

vvcephei commented Nov 11, 2019

Copy link
Copy Markdown
Contributor

(doing it again so we can capture the test results)

Retest this, please.

@vvcephei

vvcephei commented Nov 11, 2019

Copy link
Copy Markdown
Contributor

Note that it's too late for the 2.4.0 release, as of the beginning of October, so I think we need to wait for the 2.4.0 release to actually happen before cherry-picking to the 2.4 branch.

We could cherry-pick it to the 2.3 branch, but it might be safer to do the 2.4 and 2.3 cherry-picks at the same time (so we don't drop 2.4 on the floor).

@kokachev

kokachev commented Nov 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@vvcephei ,
Thanks for getting back on this.
I think it's relatively low risk patch, because it adds new functions without changing existing code. It fixes a "bug" of Scala API being unjustly treated as inferior compared to Java API :-) The only thing that can potentially fail is tests.

@bbejeck
bbejeck merged commit 9a125a7 into apache:trunk Nov 12, 2019
@bbejeck

bbejeck commented Nov 12, 2019

Copy link
Copy Markdown
Member

Merged #7520 into trunk.

@bbejeck

bbejeck commented Nov 12, 2019

Copy link
Copy Markdown
Member

Thanks for the contribution @kokachev!

@bbejeck

bbejeck commented Nov 12, 2019

Copy link
Copy Markdown
Member

FWIW I agree with @vvcephei with the cherry-picking, IMHO it's better to wait and do them at one time.

bbejeck
bbejeck reviewed Nov 12, 2019
View reviewed changes
Comment thread ...streams-scala/src/main/scala/org/apache/kafka/streams/scala/FunctionsCompatConversions.scala
Comment on lines +131 to +133
override def transform(key: K, value: V): JIterable[VO] = supplier.get().transform(key, value).asJava
override def init(context: ProcessorContext): Unit = supplier.get().init(context)
override def close(): Unit = supplier.get().close()

@bbejeck bbejeck Nov 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@kokachev I just realized that we have multiple calls to supplier.get() here. Calling supplier.get() will return a new instance each time correct? This needs to get changed here and below, as well. We can do this in a follow-up PR using KAFKA-9011.

@kokachev kokachev mentioned this pull request Nov 13, 2019
Merged
3 tasks
bbejeck pushed a commit that referenced this pull request Nov 14, 2019
@kokachev @bbejeck
KAFKA-9011: Removed multiple calls to supplier.get() in order to avoi…
7f5c380 
…d multiple transformer instances being created. (#7685)

This is a followup PR for #7520 to address issue of multiple calls to get() as it was pointed out by @bbejeck in #7520 (comment)

Reviewers: Bill Bejeck <bbejeck@gmail.com>
vvcephei pushed a commit that referenced this pull request Dec 21, 2019
@kokachev @vvcephei
KAFKA-9011: Scala bindings for flatTransform and flatTransformValues …
adeb5f7 
…in KStream (#7520)

Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bbejeck@gmail.com>
vvcephei pushed a commit that referenced this pull request Dec 21, 2019
@kokachev @vvcephei
KAFKA-9011: Removed multiple calls to supplier.get() in order to avoi…
2bcfbe2 
…d multiple transformer instances being created. (#7685)

This is a followup PR for #7520 to address issue of multiple calls to get() as it was pointed out by @bbejeck in #7520 (comment)

Reviewers: Bill Bejeck <bbejeck@gmail.com>
qq619618919 pushed a commit to qq619618919/kafka that referenced this pull request May 12, 2020
@kokachev @qq619618919
KAFKA-9011: Scala bindings for flatTransform and flatTransformValues …
203cf1d 
…in KStream (apache#7520)

Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bbejeck@gmail.com>
qq619618919 pushed a commit to qq619618919/kafka that referenced this pull request May 12, 2020
@kokachev @qq619618919
KAFKA-9011: Removed multiple calls to supplier.get() in order to avoi…
e08d76b 
…d multiple transformer instances being created. (apache#7685)

This is a followup PR for apache#7520 to address issue of multiple calls to get() as it was pointed out by @bbejeck in apache#7520 (comment)

Reviewers: Bill Bejeck <bbejeck@gmail.com>
@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

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

@kokachev @mjsax @vvcephei @bbejeck