Skip to content

KAKFA-9612 CLI Dynamic Configuration with file input#8184

Closed
WanderingStar wants to merge 6 commits into
apache:trunkfrom
WanderingStar:KAFKA-9612
Closed

KAKFA-9612 CLI Dynamic Configuration with file input#8184
WanderingStar wants to merge 6 commits into
apache:trunkfrom
WanderingStar:KAFKA-9612

Conversation

@WanderingStar

@WanderingStar WanderingStar commented Feb 27, 2020

Copy link
Copy Markdown
Contributor

Add an option to kafka-configs.sh --add-config-file that adds the configs from a properties file.
Testing: Added new tests to ConfigCommandTest.scala

Committer Checklist (excluded from commit message)

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

@dajac dajac 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. I have left few comments.

Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala Outdated

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

@WanderingStar I just made a pass over it and it looks good to me, assuming that all the tests pass. I just left one nit.

Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
@WanderingStar

Copy link
Copy Markdown
Contributor Author

@rajinisivaram, @cmccabe, @gwenshap would you mind taking a look at this PR?

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

@WanderingStar Thanks for the PR. left few comments.
also please rebase the PR.

Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala Outdated
Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala Outdated
@WanderingStar

Copy link
Copy Markdown
Contributor Author

Thanks for your review, @omkreddy! I've rebased and incorporated your feedback.

@omkreddy

Copy link
Copy Markdown
Contributor

ok to test

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

@WanderingStar Thanks for the PR. LGTM.

@omkreddy

Copy link
Copy Markdown
Contributor

JDK 8, 11 tests passed. JDK 14 test failures are not related. Merging to trunk.

@omkreddy omkreddy closed this in 2ca19cf Apr 27, 2020
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