Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kafka 4507: The client should send older versions of requests to the broker if necessary #2264

Closed
wants to merge 44 commits into from

Conversation

cmccabe
Copy link
Contributor

@cmccabe cmccabe commented Dec 16, 2016

KAFKA-4507

The client should send older versions of requests to the broker if necessary.

Note: This builds on top of KAFKA-3600, which has not yet been committed yet.

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/183/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/181/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/182/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/196/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/195/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/194/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/199/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/197/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Dec 16, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/198/
Test FAILed (JDK 8 and Scala 2.12).

@cmccabe
Copy link
Contributor Author

cmccabe commented Dec 17, 2016

Previously, there were many places in the code that were creating AbstractRequest objects directly and passing them to the NetworkClient. Unfortunately, AbstractRequest objects are version-specific. For example, a ListOffsetRequest object of version 0 will have different fields inside its Struct than a ListOffsetRequest object of version 1. As soon as the message constructor is invoked, the contained Structs are formatted in the version-specific format.

This change adds the AbstractRequest#Builder types. Client code in the Fetcher and Producer, and other places, can create these objects and pass them to the NetworkClient. The NetworkClient can then decide what version of each request to use based on the ApiVersionsRequest data. When the version that needs to be used is too old to support a necessary feature, an error is returned to the upper layer, which can then decide what to do.

@ijuma
Copy link
Contributor

ijuma commented Jan 3, 2017

@cmccabe, can you please rebase this branch? Thanks!

@cmccabe
Copy link
Contributor Author

cmccabe commented Jan 3, 2017

Rebased on trunk and fixed some unit tests

@asfbot
Copy link

asfbot commented Jan 3, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/458/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Jan 3, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/456/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Jan 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/457/
Test FAILed (JDK 8 and Scala 2.12).

@ijuma
Copy link
Contributor

ijuma commented Jan 4, 2017

Sorry, can you please rebase again?

@cmccabe
Copy link
Contributor Author

cmccabe commented Jan 4, 2017

Rebased

@cmccabe
Copy link
Contributor Author

cmccabe commented Jan 4, 2017

Also removed a few places where this change duplicated KAFKA-4548, and reverted a change to the test log4j.properties file

@asfbot
Copy link

asfbot commented Jan 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/493/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Jan 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/492/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Jan 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/491/
Test FAILed (JDK 7 and Scala 2.10).

@cmccabe
Copy link
Contributor Author

cmccabe commented Jan 4, 2017

Fix AuthorizerIntegrationTest, FetchRequestTest, SaslApiVersionsRequestTest, PlaintextConsumerTest.

Fixed a bug where Fetcher#beginningOffsets and Fetcher#endOffsets incorrectly required a broker with the new ListOffsetsRequest RPC. Will add this to CompatibilityTest.

@asfbot
Copy link

asfbot commented Jan 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/501/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Jan 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/500/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Jan 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/742/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Jan 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/740/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Jan 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/740/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Jan 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/751/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Jan 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/749/
Test PASSed (JDK 8 and Scala 2.12).

Copy link
Contributor

@ijuma ijuma 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 changes, LGTM. We can address remaining issues in follow-ups. @cmccabe, please file a JIRA and link it from here when you can.

For the record, we had a known transient system test failure (ZooKeeperSecurityUpgradeTest.test_zk_security_upgrade), but it passed on rerun.

I fixed some generic unchecked warnings before merging to trunk.

@asfgit asfgit closed this in 3d60f1e Jan 11, 2017
@asfbot
Copy link

asfbot commented Jan 11, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/749/
Test FAILed (JDK 7 and Scala 2.10).

soenkeliebau pushed a commit to soenkeliebau/kafka that referenced this pull request Feb 7, 2017
The client should send older versions of requests to the broker if necessary.

Author: Colin P. Mccabe <cmccabe@confluent.io>

Reviewers: Jason Gustafson <jason@confluent.io>, Jun Rao <junrao@gmail.com>, Ismael Juma <ismael@juma.me.uk>

Closes apache#2264 from cmccabe/KAFKA-4507
@cmccabe cmccabe deleted the KAFKA-4507 branch May 20, 2019 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants