Skip to content

KAFKA-10697: Remove ProduceResponse.responses#10332

Merged
chia7712 merged 4 commits intoapache:trunkfrom
tang7526:kafka-10697
Mar 19, 2021
Merged

KAFKA-10697: Remove ProduceResponse.responses#10332
chia7712 merged 4 commits intoapache:trunkfrom
tang7526:kafka-10697

Conversation

@tang7526
Copy link
Copy Markdown
Contributor

@tang7526 tang7526 commented Mar 16, 2021

issue : https://issues.apache.org/jira/browse/KAFKA-10697

Committer Checklist (excluded from commit message)

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

@tang7526
Copy link
Copy Markdown
Contributor Author

tang7526 commented Mar 17, 2021

@chia7712 Could you help review this PR? Thanks.

chia7712
chia7712 reviewed Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@tang7526 thanks for your patch. one major comment is left.

Comment thread clients/src/test/java/org/apache/kafka/common/requests/ProduceResponseTest.java Outdated
chia7712
chia7712 reviewed Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@tang7526 thanks for this patch. It address the great cleanup! Please take a look at following comments :)

Comment thread core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala Outdated
Comment thread core/src/test/scala/integration/kafka/network/DynamicConnectionQuotaTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/EdgeCaseRequestTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/KafkaApisTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ProduceRequestTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ProduceRequestTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ProduceRequestTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ProduceRequestTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/server/ProduceRequestTest.scala Outdated
Comment thread clients/src/test/java/org/apache/kafka/common/requests/ProduceResponseTest.java Outdated
chia7712
chia7712 reviewed Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@tang7526 thanks for updating code. a couple of comments. PTAL

Comment thread clients/src/test/java/org/apache/kafka/common/requests/ProduceResponseTest.java Outdated
Copy link
Copy Markdown
Member

@chia7712 chia7712 Mar 17, 2021

Choose a reason for hiding this comment

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

Could you use assertNull?

Copy link
Copy Markdown
Contributor Author

@tang7526 tang7526 Mar 18, 2021

Choose a reason for hiding this comment

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

Done.

Comment thread clients/src/test/java/org/apache/kafka/common/requests/ProduceResponseTest.java Outdated
Copy link
Copy Markdown
Member

@chia7712 chia7712 Mar 17, 2021

Choose a reason for hiding this comment

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

Could you add similar checks?

Copy link
Copy Markdown
Contributor Author

@tang7526 tang7526 Mar 18, 2021

Choose a reason for hiding this comment

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

Done.

chia7712
chia7712 approved these changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@tang7526 thanks for your patch. +1 after addressing latest comment :)

Comment thread clients/src/test/java/org/apache/kafka/common/requests/ProduceResponseTest.java Outdated
assertEquals(10, v2Response.throttleTimeMs(), "Throttle time must be 10");

List<ProduceResponse> arrResponse = Arrays.asList(v0Response, v1Response, v2Response);
for(ProduceResponse produceResponse:arrResponse) {
Copy link
Copy Markdown
Member

@chia7712 chia7712 Mar 18, 2021

Choose a reason for hiding this comment

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

code style: produceResponse : arrResponse

Copy link
Copy Markdown
Contributor Author

@tang7526 tang7526 Mar 18, 2021

Choose a reason for hiding this comment

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

Done.

@chia7712 chia7712 merged commit 4abdb59 into apache:trunk Mar 19, 2021
ijuma added a commit to confluentinc/kafka that referenced this pull request Mar 19, 2021
@ijuma
CONFLUENT: Sync from apache/kafka/trunk (19 March 2021)
d38bc38 
Conflicts:
* build.gradle: keep `dependencySubstitution` Confluent addition in
`resolutionStrategy` and take upstream changes.

Commits:
* apache-github/trunk:
  KAFKA-12503: inform threads to resize their cache instead of doing so for them (apache#10356)
  KAFKA-10697: Remove ProduceResponse.responses (apache#10332)
  MINOR: Exclude KIP-500.md from rat check (apache#10354)
  MINOR: Move `configurations.all` to be a child of `allprojects` (apache#10349)
  MINOR: Remove use of `NoSuchElementException` in `KafkaMetadataLog` (apache#10344)
  MINOR: Start the broker-to-controller channel for request forwarding (apache#10340)
  KAFKA-12382: add a README for KIP-500 (apache#10227)
  MINOR: Fix BaseHashTable sizing (apache#10334)
  KAFKA-10357: Add setup method to internal topics (apache#10317)
  MINOR: remove redundant null check when testing specified type (apache#10314)
  KAFKA-12293: Remove JCenter from buildscript and delete buildscript.gradle
  KAFKA-12491: Make rocksdb an `api` dependency for `streams` (apache#10341)
  KAFKA-12454: Add ERROR logging on kafka-log-dirs when given brokerIds do not  exist in current kafka cluster (apache#10304)
  KAFKA-12459; Use property testing library for raft event simulation tests (apache#10323)
  MINOR: fix failing ZooKeeper system tests (apache#10297)
  MINOR: fix client_compatibility_features_test.py (apache#10292)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tang7526 @chia7712