Skip to content

KAFKA-7658: Follow up to original PR#8027

Merged
mjsax merged 3 commits intoapache:trunkfrom
mjsax:kafka-7658-totable-followup
Feb 9, 2020
Merged

KAFKA-7658: Follow up to original PR#8027
mjsax merged 3 commits intoapache:trunkfrom
mjsax:kafka-7658-totable-followup

Conversation

@mjsax
Copy link
Copy Markdown
Member

@mjsax mjsax commented Jan 31, 2020 •
edited
Loading

  • add more unit tests
  • fix bug for auto-repartitioning
  • some code cleanup

Follow up to PR #7985. Call for review @vvcephei @highluck

For PR must be cherry-picked to 2.5 branch.

@mjsax mjsax added the streams label Jan 31, 2020
mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/AbstractStream.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Renaming this variable in this class and all its sub-classed to make it more clear what it is for

Copy link
Copy Markdown
Contributor

@highluck highluck Feb 1, 2020

Choose a reason for hiding this comment

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

I agree with this
"subTopologySourceNodes" is more clearly named

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImpl.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Creating this MaterializedInternal was actually a bug as if the passed in "store name" was null, no store name would be generated, and we would fail with an NPE. It's fixed implicitly with the refactoring (this bug was exposed after I rewrote a test -- leave a comment there, too)

Copy link
Copy Markdown
Contributor

@highluck highluck Feb 1, 2020

Choose a reason for hiding this comment

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

oh! this is good
thank you,
It was my mistake

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImplJoin.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Just some cleanup to get rid or warnings.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamImplJoin.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

The is nothing to be deprecated -- also the comment is confusing as maintainMs / segmentInterval are not use in this method

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Adding couple of new test to verify we don't accept nulls.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

This line was added in the original PR for no reason -- just removing it again.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Making the test a little bit fancier.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Changing this test, to also change the key type to make sure we can pass in serdes correctly.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Overwrite key-serde to match type (this was exposing the "store name == null" bug).

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Due to the refactoring of the KStreamImple#toTable() overloads, we get different names -- but as nothing is released yet, there is backward compatibility concern.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

This check is redundant, because we know there is only one sub-topology (ie, copartitionGroup) from the topologyDescription check above.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

Just a code simplification -- no need to not use props similarly to the other tests.

mjsax
mjsax commented Jan 31, 2020
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamImplTest.java Outdated
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 31, 2020

Choose a reason for hiding this comment

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

This is an actual fix in the test code.

Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang Feb 8, 2020

Choose a reason for hiding this comment

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

This looks better than what I did, go for it!

My original hotfix PR is just to unblock the JDK11 jenkins job.

@highluck
Copy link
Copy Markdown
Contributor

highluck commented Feb 1, 2020 •
edited
Loading

LGTM
Thank you for PR @mjsax

Thank you for fixing the shortcomings. :)

@mjsax mjsax mentioned this pull request Feb 1, 2020
Merged
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 2, 2020

Java 11 timed out.

Java8: kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdWithMixOfSuccessAndError

We can retrigger a test after a review.

@mjsax mjsax mentioned this pull request Feb 2, 2020
Merged
3 tasks
@lkokhreidze lkokhreidze mentioned this pull request Feb 4, 2020
Merged
3 tasks
highluck
highluck approved these changes Feb 4, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@highluck highluck left a comment

Choose a reason for hiding this comment

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

LGTM !

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

@highluck highluck Feb 1, 2020

Choose a reason for hiding this comment

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

oh! this is good
thank you,
It was my mistake

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

@highluck highluck Feb 1, 2020

Choose a reason for hiding this comment

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

I agree with this
"subTopologySourceNodes" is more clearly named

vvcephei
vvcephei approved these changes Feb 5, 2020
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 follow up, @mjsax . Looks good to me.

@vvcephei
Copy link
Copy Markdown
Contributor

vvcephei commented Feb 5, 2020

Test results are gone.

Retest this, please.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 5, 2020

Java 11:

kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup
kafka.api.SaslPlaintextConsumerTest.testCoordinatorFailover

Java 8:

kafka.admin.DeleteConsumerGroupsTest.testDeleteCmdWithMixOfSuccessAndError
kafka.admin.DescribeConsumerGroupTest.testDescribeGroupMembersWithShortInitializationTimeout
kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsExportImportPlan
org.apache.kafka.streams.integration.LagFetchIntegrationTest.shouldFetchLagsDuringRestoration

Retest this please.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 6, 2020

Spotbug failure -- unclear why -- could not reproduce locally -- rebased and pushed a new version just in case.

@mjsax mjsax force-pushed the kafka-7658-totable-followup branch from 048a738 to d953f70 Compare February 6, 2020 21:46
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 7, 2020

Not sure why, but it seems the build was never scheduled correctly.

Retest this please

@mjsax mjsax force-pushed the kafka-7658-totable-followup branch from d953f70 to 295398d Compare February 8, 2020 03:59
@mjsax mjsax mentioned this pull request Feb 8, 2020
Merged
3 tasks
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 8, 2020

Rebased this PR to resolve merge conflicts.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 8, 2020

Java 11 passed.

Java 8:

kafka.admin.DescribeConsumerGroupTest.testDescribeGroupMembersWithShortInitializationTimeout
kafka.admin.DescribeConsumerGroupTest.testDescribeGroupWithShortInitializationTimeout

Retest this please.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 9, 2020

Java 11 timed out. Java 8 passed. Hence, we got one green build each. Merging this now.

@mjsax mjsax merged commit 059a81e into apache:trunk Feb 9, 2020
@mjsax mjsax deleted the kafka-7658-totable-followup branch February 9, 2020 05:39
mjsax added a commit that referenced this pull request Feb 9, 2020
@mjsax
KAFKA-7658: Follow up to original PR (#8027)
536805d 
Follow up to original PR #7985 for KIP-523 (adding `KStream#toTable()` operator)
  - improve JavaDocs
  - add more unit tests
  - fix bug for auto-repartitioning
  - some code cleanup

Reviewers: High Lee <yello1109@daum.net>, John Roesler <john@confluent.io>
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 9, 2020

Merged to trunk and cherry-picked to 2.5.

stanislavkozlovski pushed a commit to stanislavkozlovski/kafka that referenced this pull request Feb 18, 2020
@mjsax @stanislavkozlovski
KAFKA-7658: Follow up to original PR (apache#8027)
cb1aa5a 
Follow up to original PR apache#7985 for KIP-523 (adding `KStream#toTable()` operator)
  - improve JavaDocs
  - add more unit tests
  - fix bug for auto-repartitioning
  - some code cleanup

Reviewers: High Lee <yello1109@daum.net>, 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

Reviewers

@guozhangwang guozhangwang guozhangwang left review comments

@vvcephei vvcephei vvcephei approved these changes

+1 more reviewer

@highluck highluck highluck approved these changes

Reviewers whose approvals may not affect merge requirements

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

@mjsax @highluck @vvcephei @guozhangwang