Skip to content

MINOR: don't log config during unit tests#5671

Merged
mjsax merged 1 commit into
apache:trunkfrom
vvcephei:quiet-config-for-unit-tests
Sep 30, 2018
Merged

MINOR: don't log config during unit tests#5671
mjsax merged 1 commit into
apache:trunkfrom
vvcephei:quiet-config-for-unit-tests

Conversation

@vvcephei

Copy link
Copy Markdown
Contributor

The StreamsConfig class by default prints its configuration values during construction.

This is valuable at runtime as a record of the actual configuration, but during unit testing,
it results in excessive output.

Committer Checklist (excluded from commit message)

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

@vvcephei

Copy link
Copy Markdown
Contributor Author

/cc @mjsax @bbejeck

@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.

@vvcephei thanks for looking into this.

I like setting the configs of during testing, but I think we can turn off logging during testing without the new class.

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.

Could we do final StreamsConfig streamsConfig = new StreamsConfig(config, false); instead? I tried locally and it worked.

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.

That works for TopologyTestDriver, but not MockProcessorContext because that constructor is protected, and the TopologyTestDriver happens to be in the same package as StreamsConfig.

I previously had the QuietStreamsConfig used only in MockProcessorContext, but the feedback was to also use it in TopologyTestDriver.

Seeing it this way, do you think it makes sense just to use the protected constructor in TopologyTestDriver, and the QuietStreamsConfig in the MockProcessorContext?

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.

With the additional context, I'm fine with the PR as is.

@vvcephei

Copy link
Copy Markdown
Contributor Author

Trunk should be fixed now.

Retest this, please.

@bbejeck

bbejeck commented Sep 24, 2018

Copy link
Copy Markdown
Member

failure unrelated

restest this please

@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.

LGTM

@vvcephei

Copy link
Copy Markdown
Contributor Author

Thanks, @bbejeck !

@mjsax mjsax 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.

One nit. Overall LGTM.

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.

Why do we need an additional package? org.apache.kafka.streams.internals seems sufficient.

@vvcephei

Copy link
Copy Markdown
Contributor Author

@mjsax I've moved the class as you suggested and also rebased on current trunk.

@mjsax mjsax 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.

LGTM

@mjsax
mjsax merged commit 49b5206 into apache:trunk Sep 30, 2018
@vvcephei
vvcephei deleted the quiet-config-for-unit-tests branch October 1, 2018 14:44
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>
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.

3 participants