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

Jump to bottom

KAFKA-4453 : Added code to separate controller connections and requests from the data plane #5921

Merged
merged 3 commits into from Jan 13, 2019
Merged

KAFKA-4453 : Added code to separate controller connections and requests from the data plane #5921

merged 3 commits into from Jan 13, 2019

Conversation

MayureshGharat
Copy link
Contributor

KIP-291 Implementation : Added code to separate controller connections and requests from the data plane.

  • Tested with local deployment that the controller request are handled by the control plane and other requests are handled by the data plane.
  • Also added unit tests in order to test the functionality.

Committer Checklist (excluded from commit message)

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

@gitlw gitlw mentioned this pull request Nov 16, 2018
Closed
3 tasks
@MayureshGharat
Copy link
Contributor Author

@junrao @jjkoshy would you mind taking a look?

junrao
junrao reviewed Dec 4, 2018
View reviewed changes
Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@MayureshGharat : Thanks for the patch. Added a few comments below.

It may be useful to add a unit test to make sure that the controller is indeed using the control plane channel specified.

core/src/main/scala/kafka/server/KafkaServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/network/SocketServerTest.scala Show resolved Hide resolved
core/src/test/scala/unit/kafka/network/SocketServerTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/network/SocketServerTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/network/SocketServerTest.scala Outdated Show resolved Hide resolved
@MayureshGharat MayureshGharat force-pushed the KAFKA-4453 branch 2 times, most recently from 7cc2627 to 983b0f4 Compare December 6, 2018 23:16
@MayureshGharat
Copy link
Contributor Author

@junrao Thanks a lot for the feedback. I have updated the PR with the changes and test to check that the controller is indeed using the control-plane if specified. Can you take another look? Thanks again.

@MayureshGharat
Copy link
Contributor Author

@jjkoshy ping

@ijuma
Copy link
Contributor

ijuma commented Dec 9, 2018

@MayureshGharat To make reviews easier, please don't squash the changes to address feedback on the original commit. Add them as additional commits so that reviewers can more easily see what has changed since the last review. Thanks!

@MayureshGharat
Copy link
Contributor Author

@ijuma my bad. I actually messed up my local git branch, so had to redo the patch locally. Will take care next time.

junrao
junrao reviewed Dec 14, 2018
View reviewed changes
Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@MayureshGharat : Thanks for the updated patch. Just a few more minor comments.

core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/server/KafkaConfig.scala Show resolved Hide resolved
core/src/main/scala/kafka/server/KafkaConfig.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/network/SocketServerTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/network/SocketServerTest.scala Outdated Show resolved Hide resolved
@MayureshGharat
Copy link
Contributor Author

@junrao Thanks a lot for the feedback. I have updated the PR, addressing your comments. Can you take another look? Thanks again!

jjkoshy
jjkoshy reviewed Dec 21, 2018
View reviewed changes
Copy link
Contributor

@jjkoshy jjkoshy left a comment

Choose a reason for hiding this comment

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

Sorry about the delay. This looks good overall. Just have a few minor comments.

core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/main/scala/kafka/network/SocketServer.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala Outdated Show resolved Hide resolved
core/src/test/scala/unit/kafka/network/SocketServerTest.scala Show resolved Hide resolved
@MayureshGharat
Added support for separating the control-plane(controller requests) f…
b9a786a 
…rom the data-plane(client and other broker requests)
@MayureshGharat
Addressed comments on the PR.
52f1e28
@MayureshGharat
Addressed comments on the PR. Changed the metric ControlPlaneNetworkP…
c40a575 
…rocessorAvgIdlePercent to return Double.NaN, if the control-plane listener is not configured.
@MayureshGharat
Copy link
Contributor Author

@jjkoshy thanks a lot for the feedback.
I have updated the PR, addressing your comments. Can you take another look?
Thanks again !

@jjkoshy
Copy link
Contributor

jjkoshy commented Jan 10, 2019

+1
ping @junrao @ijuma in case they would like to take another look.

junrao
junrao approved these changes Jan 13, 2019
View reviewed changes
Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

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

@MayureshGharat : Thanks for the latest PR. LGTM

@junrao junrao merged commit 8afce0e into apache:trunk Jan 13, 2019
@mingaliu
Copy link
Contributor

mingaliu commented Jan 14, 2019 •
edited

After this commit, my test in Jenkin fails with the ambiguous reference:

/home/jenkins/jenkins-slave/workspace/kafka-pr-jdk11-scala2.12/core/src/test/scala/unit/kafka/network/SocketServerTest.scala:191: ambiguous reference to overloaded definition,
both method putAll in class Properties of type (x$1: java.util.Map[_, _])Unit
and  method putAll in class Hashtable of type (x$1: java.util.Map[_ <: Object, _ <: Object])Unit
match argument types (java.util.Properties)
    testProps.putAll(props)

Here is my PR #5990 and its jenkin test.
I also saw this putAll() is reported as Ambiguous reference here: scala/bug#10418.

@ijuma
Copy link
Contributor

ijuma commented Jan 14, 2019

The build wasn't green for Java 11...

ijuma added a commit to ijuma/kafka that referenced this pull request Jan 14, 2019
@ijuma
HOTFIX: Fix Properties.putAll compiler error when compiling with Java 11
64e7320 
Introduced by apache#5921.
@ijuma ijuma mentioned this pull request Jan 14, 2019
Merged
3 tasks
@ijuma
Copy link
Contributor

ijuma commented Jan 14, 2019

Fix #6140

@junrao
Copy link
Contributor

junrao commented Jan 15, 2019

Thanks for the fix, @ijuma .

@ijuma ijuma mentioned this pull request Feb 21, 2019
Closed
Pengxiaolong pushed a commit to Pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@MayureshGharat @Pengxiaolong
KAFKA-4453 : Added code to separate controller connections and reques…
c6b0cd6 
…ts from the data plane (apache#5921)

KIP-291 Implementation : Added code to separate controller connections and requests from the data plane.

Tested with local deployment that the controller request are handled by the control plane and other requests are handled by the data plane.
Also added unit tests in order to test the functionality.

Author: Lucas Wang <luwang@linkedin.com>, 
Author: Mayuresh Gharat <gharatmayuresh15@gmail.com>

Reviewers: Joel Koshy <jjkoshy@gmail.com>, Jun Rao <junrao@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjkoshy jjkoshy jjkoshy left review comments

@junrao junrao junrao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@MayureshGharat @ijuma @jjkoshy @mingaliu @junrao