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.

Sign up for GitHub

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

Jump to bottom

KAFKA-8056: Use automatic RPC generation in FindCoordinator #6408

Merged
merged 4 commits into from
May 6, 2019
Merged

KAFKA-8056: Use automatic RPC generation in FindCoordinator #6408

merged 4 commits into from
May 6, 2019

Conversation

mimaison
Copy link
Member

@mimaison mimaison commented Mar 8, 2019

Committer Checklist (excluded from commit message)

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

@mimaison
Copy link
Member Author

@cmccabe @omkreddy If you have time, can you take a look?

@mimaison mimaison force-pushed the find-coordinator branch 2 times, most recently from 6ccb34d to ca9ec82 Compare April 5, 2019 14:11
@mimaison
Copy link
Member Author

retest please

@mimaison
Copy link
Member Author

retest this please

@mimaison
Copy link
Member Author

ping @cmccabe @omkreddy =)

hachikuji
hachikuji reviewed Apr 27, 2019
View reviewed changes
Copy link
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, looks good. Just a few minor comments.

clients/src/main/java/org/apache/kafka/common/requests/FindCoordinatorRequest.java Outdated Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/FindCoordinatorResponse.java Outdated Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java Outdated Show resolved Hide resolved
clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java Outdated Show resolved Hide resolved
core/src/main/scala/kafka/server/KafkaApis.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/server/KafkaApis.scala
@@ -1179,8 +1180,18 @@ class KafkaApis(val requestChannel: RequestChannel,
}

def createResponse(requestThrottleMs: Int): AbstractResponse = {
def createFindCoordinatorResponse(error: Errors, node: Node): FindCoordinatorResponse = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably modify prepareResponse to take the throttle time and remove this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The throttle time is only used in 2 places so I kept it this way. I'm happy to change it if you think that's really better

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong preference. Just a suggestion

core/src/test/scala/unit/kafka/server/RequestQuotaTest.scala Outdated Show resolved Hide resolved
@mimaison
Copy link
Member Author

Thanks @hachikuji for the review. I've pushed an update to address your feedback.
Once you're happy with the changes, I'll rebase on top of trunk

hachikuji
hachikuji approved these changes May 1, 2019
View reviewed changes
Copy link
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 (assuming tests pass after rebasing).

@mimaison
Copy link
Member Author

mimaison commented May 4, 2019

retest this please

@hachikuji
Copy link
Contributor 

06:10:09 > Task :clients:compileTestJava
06:10:09 /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk11-scala2.12/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java:665: error: incompatible types: Errors cannot be converted to Struct
06:10:09         client.prepareResponse(new FindCoordinatorResponse(Errors.NONE, host1));
06:10:09

@mimaison
Copy link
Member Author

mimaison commented May 6, 2019

@hachikuji I wonder if the rebase/push force confused Jenkins because this code is not in my branch, KafkaProducerTest is using the new method already, see https://github.com/mimaison/kafka/blob/find-coordinator/clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java

@hachikuji
Copy link
Contributor

retest this please

@hachikuji
Copy link
Contributor

@mimaison I didn't see the errors when I checked out locally, so let's give jenkins another spin.

@hachikuji
Copy link
Contributor

I hadn't actually rebased when I checked. I reproduced the error after doing so and pushed a fix.

@hachikuji
Copy link
Contributor

The test failure looks unrelated. I will go ahead and merge.

@hachikuji hachikuji merged commit 407bcdf into apache:trunk May 6, 2019
@mimaison
Copy link
Member Author

mimaison commented May 7, 2019

Thank you!

ijuma added a commit to ijuma/kafka that referenced this pull request May 8, 2019
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into improve-equal…
2b8ab4d 
…s-hashcode

* apache-github/trunk:
  KAFKA-8158: Add EntityType for Kafka RPC fields (apache#6503)
  MINOR: correctly parse version OffsetCommitResponse version < 3
  KAFKA-8284: enable static membership on KStream (apache#6673)
  KAFKA-8304: Fix registration of Connect REST extensions (apache#6651)
  KAFKA-8275; Take throttling into account when choosing least loaded node (apache#6619)
  KAFKA-3522: Interactive Queries must return timestamped stores (apache#6661)
  MINOR: MetricsIntegrationTest should set StreamsConfig.STATE_DIR_CONFIG (apache#6687)
  MINOR: Remove unused field in `ListenerConnectionQuota`
  KAFKA-8131; Move --version implementation into CommandLineUtils (apache#6481)
  KAFKA-8056; Use automatic RPC generation for FindCoordinator (apache#6408)
  MINOR: Remove workarounds for lz4-java bug affecting byte buffers (apache#6679)
  KAFKA-7455: Support JmxTool to connect to a secured RMI port. (apache#5968)
  MINOR: Document improvement (apache#6682)
  MINOR: Fix ThrottledReplicaListValidator doc error. (apache#6537)
  KAFKA-8306; Initialize log end offset accurately when start offset is non-zero (apache#6652)
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@mimaison @pengxiaolong
KAFKA-8056; Use automatic RPC generation for FindCoordinator (apache#…
5b5a961 
…6408)

Reviewers: 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

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
@mimaison @hachikuji