Skip to content

KAFKA-10147 MockAdminClient#describeConfigs(Collection<ConfigResource…#8853

Merged
rhauch merged 1 commit intoapache:trunkfrom
chia7712:KAFKA-10147
Jun 17, 2020
Merged

KAFKA-10147 MockAdminClient#describeConfigs(Collection<ConfigResource…#8853
rhauch merged 1 commit intoapache:trunkfrom
chia7712:KAFKA-10147

Conversation

@chia7712
Copy link
Copy Markdown
Member

MockAdminClient#describeConfigs(Collection<ConfigResource>) has new implementation introduced by 48b56e5. It is unable to handle broker resource so ReassignPartitionsUnitTest#testModifyBrokerThrottles throws NPE.

https://issues.apache.org/jira/browse/KAFKA-10147

Committer Checklist (excluded from commit message)

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

@chia7712
Copy link
Copy Markdown
Member Author

@rhauch Could you take a look?

@rhauch
Copy link
Copy Markdown
Contributor

rhauch commented Jun 11, 2020

Yeah, will look more closely in a few min. If this breaks tests, can you please change the KAFKA issue priority to blocker and fill in the affected versions?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe just deprecate public method instead of removing it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this method has default implementation (see https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/admin/Admin.java#L340) which calls the variety describeConfigs(Collection<ConfigResource>, DescribeConfigsOptions) so it is ok to remove the new implementation.

Comment thread clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java Outdated
@mumrah
Copy link
Copy Markdown
Member

mumrah commented Jun 12, 2020

retest this please

1 similar comment
@rhauch
Copy link
Copy Markdown
Contributor

rhauch commented Jun 12, 2020

retest this please

@chia7712
Copy link
Copy Markdown
Member Author

the failed build is traced by #8857

@chia7712
Copy link
Copy Markdown
Member Author

retest this please

@chia7712
Copy link
Copy Markdown
Member Author

retest this please

@chia7712
Copy link
Copy Markdown
Member Author

chia7712 commented Jun 15, 2020

StoreUpgradeIntegrationTest#shouldMigratePersistentKeyValueStoreToTimestampedKeyValueStoreUsingPapi is traced by https://issues.apache.org/jira/browse/KAFKA-10151

other flaky pass on my local :(

@chia7712
Copy link
Copy Markdown
Member Author

retest this please

1 similar comment
@chia7712
Copy link
Copy Markdown
Member Author

retest this please

@ableegoldman
Copy link
Copy Markdown
Member

@rhauch is this ready to be merged?

Copy link
Copy Markdown
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @chia7712!

Comment on lines +535 to +537
else return new Config(topicMetadata.configs.entrySet().stream()
.map(entry -> new ConfigEntry(entry.getKey(), entry.getValue()))
.collect(Collectors.toList()));
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.

nit: I think that we can use toConfigObject(topicMetadata.configs) here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It has been merged when I notice your comment. Let me address it in another PR :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@rhauch rhauch merged commit 26e238c into apache:trunk Jun 17, 2020
rhauch pushed a commit that referenced this pull request Jun 17, 2020
…>) is unable to handle broker resource (#8853)

Author: Chia-Ping Tsai <chia7712@gmail.com>
Reviewers: Boyang Chen <boyang@confluent.io>, Randall Hauch <rhauch@gmail.com>
Kvicii pushed a commit to Kvicii/kafka that referenced this pull request Jun 21, 2020
* 'trunk' of github.com:apache/kafka:
  KAFKA-10168: fix StreamsConfig parameter name variable (apache#8865)
  MINOR: code cleanup for inconsistent naming (apache#8871)
  KAFKA-10138: Prefer --bootstrap-server for reassign_partitions command in ducktape tests (apache#8898)
  KAFKA-10185: Restoration info logging (apache#8896)
  KAFKA-9891: add integration tests for EOS and StandbyTask (apache#8890)
  MINOR: Reduce build time by gating test coverage plugins behind a flag (apache#8899)
  KAFKA-10141; Add more detail to log segment delete messages (apache#8850)
  KAFKA-10113; Specify fetch offsets correctly in `LogTruncationException` (apache#8822)
  KAFKA-10167: use the admin client to read end-offset (apache#8876)
  MINOR: Upgrade ducktape to 0.7.8 (apache#8879)
  KAFKA-10123; Fix incorrect value for AWAIT_RESET#hasPosition (apache#8841)
  KAFKA-9896: fix flaky StandbyTaskEOSIntegrationTest (apache#8883)
  MINOR: clean up unused checkstyle suppressions for Streams (apache#8861)
  MINOR: reuse toConfigObject(Map) to generate Config (apache#8889)
  MINOR: Upgrade jetty to 9.4.27.v20200227 and jersey to 2.31 (apache#8859)
  MINOR: Fix flaky HighAvailabilityTaskAssignorIntegrationTest (apache#8884)
  KAFKA-10147 MockAdminClient#describeConfigs(Collection<ConfigResource>) is unable to handle broker resource (apache#8853)
  KAFKA-10165: Remove Percentiles from e2e metrics (apache#8882)

# Conflicts:
#	core/src/main/scala/kafka/log/Log.scala
@chia7712 chia7712 deleted the KAFKA-10147 branch March 25, 2024 15:23
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.

6 participants