Skip to content

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

Merged
hachikuji merged 4 commits into
apache:trunkfrom
mimaison:find-coordinator
May 6, 2019
Merged

KAFKA-8056: Use automatic RPC generation in FindCoordinator#6408
hachikuji merged 4 commits into
apache:trunkfrom
mimaison:find-coordinator

Conversation

@mimaison

@mimaison mimaison commented Mar 8, 2019

Copy link
Copy Markdown
Member

Committer Checklist (excluded from commit message)

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

@mimaison

mimaison commented Mar 27, 2019

Copy link
Copy Markdown
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

mimaison commented Apr 18, 2019

Copy link
Copy Markdown
Member Author

retest please

@mimaison

mimaison commented Apr 24, 2019

Copy link
Copy Markdown
Member Author

retest this please

@mimaison

mimaison commented Apr 25, 2019

Copy link
Copy Markdown
Member Author

ping @cmccabe @omkreddy =)

hachikuji
hachikuji reviewed Apr 27, 2019
View reviewed changes

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread clients/src/main/java/org/apache/kafka/common/requests/FindCoordinatorRequest.java Outdated
Comment thread clients/src/main/java/org/apache/kafka/common/requests/FindCoordinatorResponse.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java Outdated
Comment thread clients/src/test/java/org/apache/kafka/clients/producer/internals/TransactionManagerTest.java Outdated
Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated
Comment thread core/src/main/scala/kafka/server/KafkaApis.scala Outdated

@hachikuji hachikuji Apr 27, 2019

Copy link
Copy Markdown
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.

@mimaison mimaison Apr 30, 2019

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.

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

@hachikuji hachikuji Apr 30, 2019

Copy link
Copy Markdown
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

Comment thread core/src/test/scala/unit/kafka/server/RequestQuotaTest.scala Outdated
@mimaison

mimaison commented Apr 30, 2019

Copy link
Copy Markdown
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

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

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 mimaison force-pushed the find-coordinator branch from cddb391 to 4a9d629 Compare May 4, 2019 11:45
@mimaison

mimaison commented May 4, 2019

Copy link
Copy Markdown
Member Author

retest this please

@hachikuji

hachikuji commented May 6, 2019

Copy link
Copy Markdown
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

mimaison commented May 6, 2019

Copy link
Copy Markdown
Member Author

@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

hachikuji commented May 6, 2019

Copy link
Copy Markdown
Contributor

retest this please

@hachikuji

hachikuji commented May 6, 2019

Copy link
Copy Markdown
Contributor

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

@hachikuji hachikuji force-pushed the find-coordinator branch from 4a9d629 to 25f2ac3 Compare May 6, 2019 19:03
@hachikuji

hachikuji commented May 6, 2019

Copy link
Copy Markdown
Contributor

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

@hachikuji

hachikuji commented May 6, 2019

Copy link
Copy Markdown
Contributor

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

@hachikuji hachikuji merged commit 407bcdf into apache:trunk May 6, 2019
@mimaison

mimaison commented May 7, 2019

Copy link
Copy Markdown
Member Author

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