-
Notifications
You must be signed in to change notification settings - Fork 15.4k
MINOR: extract jointly owned parts of BrokerServer and ControllerServer #12837
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
Changes from 1 commit
c5897b6
e7eb56c
c6c3b20
bb8fc9e
dbbda79
733004e
392d233
8b2ec40
b61a62b
c14bf56
e984df8
be080fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| import kafka.server.BrokerServer; | ||
| import kafka.server.ControllerServer; | ||
| import kafka.server.FaultHandlerFactory; | ||
| import kafka.server.JointServer; | ||
| import kafka.server.SharedServer; | ||
| import kafka.server.KafkaConfig; | ||
| import kafka.server.KafkaConfig$; | ||
| import kafka.server.KafkaRaftServer; | ||
|
|
@@ -199,7 +199,7 @@ private KafkaConfig createNodeConfig(TestKitNode node) { | |
| public KafkaClusterTestKit build() throws Exception { | ||
| Map<Integer, ControllerServer> controllers = new HashMap<>(); | ||
| Map<Integer, BrokerServer> brokers = new HashMap<>(); | ||
| Map<Integer, JointServer> jointServers = new HashMap<>(); | ||
| Map<Integer, SharedServer> jointServers = new HashMap<>(); | ||
| /* | ||
| Number of threads = Total number of brokers + Total number of controllers + Total number of Raft Managers | ||
| = Total number of brokers + Total number of controllers * 2 | ||
|
|
@@ -223,7 +223,7 @@ public KafkaClusterTestKit build() throws Exception { | |
| String threadNamePrefix = (nodes.brokerNodes().containsKey(node.id())) ? | ||
| String.format("colocated%d", node.id()) : | ||
| String.format("controller%d", node.id()); | ||
| JointServer jointServer = new JointServer(createNodeConfig(node), | ||
| SharedServer sharedServer = new SharedServer(createNodeConfig(node), | ||
| MetaProperties.apply(nodes.clusterId().toString(), node.id()), | ||
| Time.SYSTEM, | ||
| new Metrics(), | ||
|
|
@@ -233,12 +233,12 @@ public KafkaClusterTestKit build() throws Exception { | |
| ControllerServer controller = null; | ||
| try { | ||
| controller = new ControllerServer( | ||
| jointServer, | ||
| sharedServer, | ||
| KafkaRaftServer.configSchema(), | ||
| bootstrapMetadata); | ||
| } catch (Throwable e) { | ||
| log.error("Error creating controller {}", node.id(), e); | ||
| Utils.swallow(log, "jointServer.stopForController", () -> jointServer.stopForController()); | ||
| Utils.swallow(log, "sharedServer.stopForController", () -> sharedServer.stopForController()); | ||
| if (controller != null) controller.shutdown(); | ||
| throw e; | ||
| } | ||
|
|
@@ -250,11 +250,11 @@ public KafkaClusterTestKit build() throws Exception { | |
| connectFutureManager.registerPort(node.id(), port); | ||
| } | ||
| }); | ||
| jointServers.put(node.id(), jointServer); | ||
| jointServers.put(node.id(), sharedServer); | ||
| } | ||
| for (BrokerNode node : nodes.brokerNodes().values()) { | ||
| JointServer jointServer = jointServers.computeIfAbsent(node.id(), | ||
| id -> new JointServer(createNodeConfig(node), | ||
| SharedServer sharedServer = jointServers.computeIfAbsent(node.id(), | ||
| id -> new SharedServer(createNodeConfig(node), | ||
| MetaProperties.apply(nodes.clusterId().toString(), id), | ||
| Time.SYSTEM, | ||
| new Metrics(), | ||
|
|
@@ -264,11 +264,11 @@ public KafkaClusterTestKit build() throws Exception { | |
| BrokerServer broker = null; | ||
| try { | ||
| broker = new BrokerServer( | ||
| jointServer, | ||
| sharedServer, | ||
| JavaConverters.asScalaBuffer(Collections.<String>emptyList()).toSeq()); | ||
| } catch (Throwable e) { | ||
| log.error("Error creating broker {}", node.id(), e); | ||
| Utils.swallow(log, "jointServer.stopForBroker", () -> jointServer.stopForBroker()); | ||
| Utils.swallow(log, "sharedServer.stopForBroker", () -> sharedServer.stopForBroker()); | ||
| if (broker != null) broker.shutdown(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: similar comment as before. I think JointServer still has resources that need to be cleaned up even if it doesn't get started (e.g.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think But yes, we should shut down all that. Fixed. |
||
| throw e; | ||
| } | ||
|
|
@@ -503,11 +503,11 @@ public Map<Integer, BrokerServer> brokers() { | |
| public Map<Integer, KafkaRaftManager<ApiMessageAndVersion>> raftManagers() { | ||
| Map<Integer, KafkaRaftManager<ApiMessageAndVersion>> results = new HashMap<>(); | ||
| for (BrokerServer brokerServer : brokers().values()) { | ||
| results.put(brokerServer.config().brokerId(), brokerServer.jointServer().raftManager()); | ||
| results.put(brokerServer.config().brokerId(), brokerServer.sharedServer().raftManager()); | ||
| } | ||
| for (ControllerServer controllerServer : controllers().values()) { | ||
| if (!results.containsKey(controllerServer.config().nodeId())) { | ||
| results.put(controllerServer.config().nodeId(), controllerServer.jointServer().raftManager()); | ||
| results.put(controllerServer.config().nodeId(), controllerServer.sharedServer().raftManager()); | ||
| } | ||
| } | ||
| return results; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here why this needs to be a
def?