Skip to content

KAFKA-9040 Add --all that includes dynamic and static config#7607

Merged
hachikuji merged 4 commits intoapache:trunkfrom
raymondng:kafka-9040
Jan 30, 2020
Merged

KAFKA-9040 Add --all that includes dynamic and static config#7607
hachikuji merged 4 commits intoapache:trunkfrom
raymondng:kafka-9040

Conversation

@raymondng
Copy link
Copy Markdown
Contributor

@raymondng raymondng commented Oct 29, 2019

KAFKA-9040 Implement --all option for describing configs

Added unit test: kafka.admin.ConfigCommandTest.testDescribeAllBrokerConfig

Committer Checklist (excluded from commit message)

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

bdbyrne
bdbyrne reviewed Oct 29, 2019
View reviewed changes
Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
Copy link
Copy Markdown
Contributor

@bdbyrne bdbyrne Oct 29, 2019

Choose a reason for hiding this comment

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

Prefer not to refer to static and dynamic since they're specific to the broker configs (topics and quotas are all dynamic).

Copy link
Copy Markdown
Contributor Author

@raymondng raymondng Oct 29, 2019

Choose a reason for hiding this comment

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

Sounds good. I'll remove static and dynamic from the description.

Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala Outdated
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Oct 29, 2019

Thanks for the PR. Is there a KIP for this? Changes that affect user visible interfaces require a KIP.

@raymondng
Copy link
Copy Markdown
Contributor Author

raymondng commented Oct 29, 2019

Yes there is. See KIP-524 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-524%3A+Allow+users+to+choose+config+source+when+describing+configs).

@bdbyrne
Copy link
Copy Markdown
Contributor

bdbyrne commented Oct 29, 2019

Have a look at the Jenkins output and see if you can locate the failure. Not sure if you've made it this far yet, but Kafka is surprisingly easy to test locally, as you really only need to do the following (from top):

Term-1: ./bin/zookeeper-server-start.sh config/zookeeper.properties
Term-2: /bin/kafka-server-start.sh config/server.properties

And you should be up and running on localhost:9092. Running gradle test locally is a huge pain, but you can specify a single test via:

./gradlew core:test --tests=kafka.admin.ConfigCommandTest

@raymondng
Copy link
Copy Markdown
Contributor Author

raymondng commented Oct 29, 2019

Thanks @bdbyrne! Looks like the change to bootstrap-server from zookeeper entailed further tweaks to the test. That's been addressed.

@bdbyrne
Copy link
Copy Markdown
Contributor

bdbyrne commented Oct 29, 2019

LGTM

hachikuji
hachikuji reviewed Dec 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji 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 patch. Just had one comment for consideration.

Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala Outdated
@raymondng
KAFKA-9040 Add --all that includes dynamic and static config
cef3fed 
Added unit test: kafka.admin.ConfigCommandTest.testDescribeAllBrokerConfig
hachikuji
hachikuji reviewed Jan 3, 2020
View reviewed changes
Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala
@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jan 7, 2020

retest this please

@hachikuji
Copy link
Copy Markdown
Contributor

hachikuji commented Jan 29, 2020

ok to test

hachikuji
hachikuji approved these changes Jan 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM

@hachikuji hachikuji merged commit 8494fdb into apache:trunk Jan 30, 2020
tedyu
tedyu reviewed Jan 30, 2020
View reviewed changes
Comment thread core/src/main/scala/kafka/admin/ConfigCommand.scala
throw new IllegalArgumentException("Only one of --bootstrap-server or --zookeeper must be specified")

if (options.has(allOpt) && options.has(zkConnectOpt)) {
throw new IllegalArgumentException(s"--bootstrap-server must be specified for --all")
Copy link
Copy Markdown
Contributor

@tedyu tedyu Jan 30, 2020

Choose a reason for hiding this comment

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

Since the user may not have seen the 'Only one of --bootstrap-server or --zookeeper' error above, it would be clearer to mention that zkConnectOpt should be dropped while adding '--bootstrap-server'

ijuma added a commit to confluentinc/kafka that referenced this pull request Feb 2, 2020
@ijuma
CONFLUENT: Sync from apache/kafka (1 February 2020)
e52c86c 
Conflicts and/or compiler errors due to the fact that we
temporarily reverted the commit that removes
Scala 2.11 support:

* SslAdminIntegrationTest: keep using JAdminClient,
take upstream changes otherwise.
* ReassignPartitionsClusterTest: keep using
JAdminClient, take upstream changes otherwise.
* KafkaApis: use `asScala.foreach` instead of
`forEach`.

# By Ismael Juma (3) and others
# Via GitHub
* apache-github/trunk: (22 commits)
  KAFKA-9437; Make the Kafka Protocol Friendlier with L7 Proxies [KIP-559] (apache#7994)
  KAFKA-9375: Add names to all Connect threads (apache#7901)
  MINOR: Introduce 2.5-IV0 IBP (apache#8010)
  KAFKA-8503; Add default api timeout to AdminClient (KIP-533) (apache#8011)
  Add retries to release.py script (apache#8021)
  KAFKA-8162: IBM JDK Class not found error when handling SASL (apache#6524)
  MINOR: Add explicit result type in public defs/vals (apache#7993)
  KAFKA-9408: Use StandardCharsets.UTF-8 instead of "UTF-8" (apache#7940)
  KAFKA-9474: Adds 'float64' to the RPC protocol types (apache#8012)
  KAFKA-9360: Allow disabling MM2 heartbeat and checkpoint emissions (apache#7887)
  KAFKA-7658: Add KStream#toTable to the Streams DSL (apache#7985)
  KAFKA-9445: Allow adding changes to allow serving from a specific partition (apache#7984)
  KAFKA-9422: Track the set of topics a connector is using (KIP-558) (apache#8017)
  KAFKA-9040; Add --all option to config command (apache#7607)
  KAFKA-4203: Align broker default for max.message.bytes with Java producer default (apache#4154)
  KAFKA-9426: Use switch instead of chained if/else in OffsetsForLeaderEpochClient (apache#7959)
  KAFKA-9405: Use Map.computeIfAbsent where applicable (apache#7937)
  KAFKA-9026: Use automatic RPC generation in DescribeAcls (apache#7560)
  MINOR: Remove unused fields in StreamsMetricsImpl (apache#7992)
  KAFKA-9460: Enable only TLSv1.2 by default and disable other TLS protocol versions (KIP-553) (apache#7998)
  ...
gitlw pushed a commit to linkedin/kafka that referenced this pull request Sep 9, 2021
@r00ng @gitlw
[LI-CHERRY-PICK] [8494fdb] KAFKA-9040; Add --all option to config com…
232b628 
…mand (apache#7607)

Implement --all option for describing all configs (both dynamic and static) as documented in KIP-524 (https://cwiki.apache.org/confluence/display/KAFKA/KIP-524%3A+Allow+users+to+choose+config+source+when+describing+configs.

Reviewers: Brian Byrne <bbyrne@confluent.io>, Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hachikuji hachikuji hachikuji approved these changes

+2 more reviewers

@bdbyrne bdbyrne bdbyrne left review comments

@tedyu tedyu tedyu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@raymondng @ijuma @bdbyrne @hachikuji @tedyu