Skip to content

KAFKA-9483; Add Scala KStream#toTable to the Streams DSL#8024

Merged
mjsax merged 1 commit intoapache:trunkfrom
highluck:scala
Feb 11, 2020
Merged

KAFKA-9483; Add Scala KStream#toTable to the Streams DSL#8024
mjsax merged 1 commit intoapache:trunkfrom
highluck:scala

Conversation

@highluck
Copy link
Copy Markdown
Contributor

Add Scala KStream#toTable to the Streams DSL
https://issues.apache.org/jira/browse/KAFKA-9483

Committer Checklist (excluded from commit message)

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

Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM.

@vvcephei @bbejeck -- I am not fluent in Scala -- can you have a second look?

@highluck highluck requested review from vvcephei and removed request for guozhangwang February 1, 2020 00:55
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.

LGMT pending passing build

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 2, 2020

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 2, 2020

Java 11: KStreamImplTest.shouldSupportTriggerMaterializedWithKTableFromKStream (should be covered via #8027 -- did not create a ticket)

Java 8:

kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdWithMixOfSuccessAndError
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsExportImportPlan

We can trigger retesting after John's review.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 4, 2020

I just realized, that Bill review and approved already...

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 5, 2020

Both tests failed. Result are not available any longer.

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 8, 2020

I need to cycle back to this quicker. Same as before. Both tests failed but result are not available any longer.

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 9, 2020

Java 8:

kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdWithMixOfSuccessAndError

Java 11:

kafka.admin.DescribeConsumerGroupTest.testDescribeGroupWithShortInitializationTimeout

Java 8 test failure is the same as above. Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 9, 2020

@bbejeck @vvcephei @guozhangwang The Scala part of the API change was not explicitly discussed in the KIP -- do you think this is an issue and we should try to include such a discussion for all API changes?

Also, this PR is technically an API change, hence, I am wondering if it would be ok to cherry-pick to 2.5 branch or not? The feature itself was merged on time -- I personally think it would be ok to cherry-pick, but would like to double check what you think?

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 9, 2020

Java 8 timed out.
Java 11:

kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsExportImportPlan

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 10, 2020

Java 8:

kafka.admin.DescribeConsumerGroupTest.testDescribeGroupMembersWithShortInitializationTimeout

Java 11:

 kafka.admin.DeleteConsumerGroupsTest > testDeleteCmdWithMixOfSuccessAndError

I guess we can merge this?

@highluck
Copy link
Copy Markdown
Contributor Author

@mjsax
Thank you for your care!
But, No problem in my local environment

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 all, really sorry about this, but I just now noticed a significant issue in the test.

Regarding @mjsax 's earlier question, I can't believe we missed the scala API again in the KIP... We should just update the KIP and send a note to the vote thread. Also, we should cherry-pick to 2.5.

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 10, 2020

Regarding @mjsax 's earlier question, I can't believe we missed the scala API again in the KIP... We should just update the KIP and send a note to the vote thread. Also, we should cherry-pick to 2.5.

+1

@highluck
Copy link
Copy Markdown
Contributor Author

highluck commented Feb 10, 2020

@vvcephei
thanks for review !
Updated the KIP and updated the test.

retest this please

@highluck highluck requested a review from vvcephei February 10, 2020 23:15
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 @highluck

@vvcephei
Copy link
Copy Markdown
Contributor

Ok to test.

@vvcephei
Copy link
Copy Markdown
Contributor

Retest this, please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 11, 2020

Both runs passed. Merging.

@mjsax mjsax merged commit dc89c86 into apache:trunk Feb 11, 2020
mjsax pushed a commit that referenced this pull request Feb 11, 2020
Part of KIP-523

Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>
@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 11, 2020

Merged to trunk and cherry-picked to 2.5.

Thanks for the PR @highluck!

stanislavkozlovski pushed a commit to stanislavkozlovski/kafka that referenced this pull request Feb 18, 2020
Part of KIP-523

Reviewers: Matthias J. Sax <matthias@confluent.io>, Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>
@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

Labels

kip Requires or implements a KIP streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants