Skip to content

KAFKA-12749: Changelog topic config on suppressed KTable lost#10664

Merged
cadonna merged 8 commits into
apache:trunkfrom
vichu:KAFKA-12749
Jun 3, 2021
Merged

KAFKA-12749: Changelog topic config on suppressed KTable lost#10664
cadonna merged 8 commits into
apache:trunkfrom
vichu:KAFKA-12749

Conversation

@vichu

@vichu vichu commented May 11, 2021

Copy link
Copy Markdown
Contributor

Refactored logConfig to be passed appropriately when using shutDownWhenFull or emitEarlyWhenFull. Removed the constructor that doesn't accept a logConfig parameter so you're forced to specify it explicitly, whether it's empty/unspecified or not.

Ticket: https://issues.apache.org/jira/browse/KAFKA-12749

Committer Checklist (excluded from commit message)

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

@vichu

vichu commented May 11, 2021

Copy link
Copy Markdown
Contributor Author

@ableegoldman Would appreciate it if you can take a look at this PR when you get a chance.

@ableegoldman

Copy link
Copy Markdown
Member

cc any of @cadonna @vvcephei @lct45 @wcarlson5 @mjsax @guozhangwang to review this

@wcarlson5 wcarlson5 left a comment

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.

Thanks for the PR!

The implementation looks pretty good so far, but can we add some tests to make sure that configs are no longer los if either emitEarlyWhenFull or shutDownWhenFull is set after the logging config?

"keys alone should be set",
maxRecords(2L),
is(new EagerBufferConfigImpl(2L, MAX_VALUE))
is(new EagerBufferConfigImpl(2L, MAX_VALUE, Collections.emptyMap()))

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.

@vichu Can you add some tests that don't just use an empty map?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure.
Added a few tests to verify the logConfig map as well.

@wcarlson5

Copy link
Copy Markdown
Contributor

Sorry, this got lost in my inbox. @vichu if you can rebase I think its good to go. do you agree @ableegoldman ?

# Conflicts:
#	streams/streams-scala/src/main/scala/org/apache/kafka/streams/scala/kstream/Suppressed.scala
#	streams/streams-scala/src/test/scala/org/apache/kafka/streams/scala/kstream/SuppressedTest.scala

@cadonna cadonna 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 for the PR @vichu!

LGTM!

I found just one nit!

@cadonna

cadonna commented Jun 3, 2021

Copy link
Copy Markdown
Member

@vichu I took the liberty to commit the fix to the nit directly. I hope that is fine with you.

@cadonna

cadonna commented Jun 3, 2021

Copy link
Copy Markdown
Member

I will merge as soon as the checks are done.

@cadonna

cadonna commented Jun 3, 2021

Copy link
Copy Markdown
Member

The 7th build was green on the same commit. I do not know why a 8th build was started.
https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-10664/7/pipeline/15

@cadonna cadonna merged commit 93dca8e into apache:trunk Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants