[ 
https://issues.apache.org/jira/browse/KAFKA-1809?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14347236#comment-14347236
 ] 

Jun Rao commented on KAFKA-1809:
--------------------------------

Thanks for the patch. Some comments.

1. KafkaConfig:
1.1 val interBrokerSecurityProtocol => val intraBrokerSecurityProtocol
1.2 In the getListeners() and getAdvertisedListeners(), let's use the same 
default port 9092 as before.
      Utils.listenerListToEndPoints("PLAINTEXT://" + 
props.getString("advertised.host.name", props.getString("host.name", ""))
              + ":" + props.getInt("advertised.port", props.getInt("port", 
6667)).toString)
1.3 In the comment,
lets use them => let's use them
1.4 In the following comment, those explicitly enumerated versions may get out 
of date. We can just point to ApiVersion for valid values.
   * Valid values are: 0.8.2.0, 0.8.3.0
1.5 We probably should validate that all specified security protocols are valid 
(i.e., those defined in SecurityProtocol)

2. ApiVersion: Instead of 0.8.2.0, should we just use 0.8.2 since the protocol 
for 0.8.2.0 and 0.8.2.1 are the same?

3. EndPoint:  For the following, could we give a plain english description of 
the uri format a and give a few examples?
    val uriParseExp = """^(.*)://\[?([0-9a-z\-.:]*)\]?:([0-9]+)""".r

4. AdminUtils: In the following, no need to do id => id.int since id is already 
int.
            replicaInfo = getBrokerInfoFromCache(zkClient, cachedBrokerInfo, 
replicas.map(id => id.toInt)).map(_.getBrokerEndPoint(protocol))

5. Broker: In the following, instead of representing endpoints as a single 
string, should we represent it as an array of strings, one for each endpoint?
 *  "endpoints": "{PLAINTEXT://host1:9092,SSL://host1:9093"}

6. ConsumerConfig: Do we need to expose the following config? Not sure how it's 
used in testing.
  val securityProtocol = 
SecurityProtocol.withName(props.getString("security.protocol", "PLAINTEXT"))

7. unused import: FetchTest, KafkaConfigTest(under server), 
KafkaServerTestHarness, ProducerSendTest, RequestResponseSerializationTest, 
SocketServer

8. KafkaConfigTest: Can isValidKafkaConfig be private?

9. KafkaServer: Why is the getBrokerId() call moved after the creation of 
SocketServer?
      socketServer = new SocketServer(config.brokerId,
                                      config.listeners,
                                      config.numNetworkThreads,
                                      config.queuedMaxRequests,
                                      config.socketSendBufferBytes,
                                      config.socketReceiveBufferBytes,
                                      config.socketRequestMaxBytes,
                                      config.maxConnectionsPerIp,
                                      config.connectionsMaxIdleMs,
                                      config.maxConnectionsPerIpOverrides)
        socketServer.startup()

        /* generate brokerId */
        config.brokerId =  getBrokerId
        this.logIdent = "[Kafka Server " + config.brokerId + "], "

10. SecurityProtocol: When will TRACE be used? It's not used in ProducerConfig.

11. Some of the files have extra empty lines (e.g. SocketServerTest)

12. testcase_1_properties.json: Could you explain why we need to change the log 
segment size?

13. UpdateMetadataRequest: sizeInBytes() needs to be versioned as well. Is 
there no unit test covering this? If so, we should add one.

14. Testing.
14.1 Did you run the system tests?
14.2 Did you try running existing tools?
14.3 How should we test the protocol upgrade from 0.8.2 to 0.8.3?


> Refactor brokers to allow listening on multiple ports and IPs 
> --------------------------------------------------------------
>
>                 Key: KAFKA-1809
>                 URL: https://issues.apache.org/jira/browse/KAFKA-1809
>             Project: Kafka
>          Issue Type: Sub-task
>          Components: security
>            Reporter: Gwen Shapira
>            Assignee: Gwen Shapira
>         Attachments: KAFKA-1809.patch, KAFKA-1809.v1.patch, 
> KAFKA-1809.v2.patch, KAFKA-1809.v3.patch, 
> KAFKA-1809_2014-12-25_11:04:17.patch, KAFKA-1809_2014-12-27_12:03:35.patch, 
> KAFKA-1809_2014-12-27_12:22:56.patch, KAFKA-1809_2014-12-29_12:11:36.patch, 
> KAFKA-1809_2014-12-30_11:25:29.patch, KAFKA-1809_2014-12-30_18:48:46.patch, 
> KAFKA-1809_2015-01-05_14:23:57.patch, KAFKA-1809_2015-01-05_20:25:15.patch, 
> KAFKA-1809_2015-01-05_21:40:14.patch, KAFKA-1809_2015-01-06_11:46:22.patch, 
> KAFKA-1809_2015-01-13_18:16:21.patch, KAFKA-1809_2015-01-28_10:26:22.patch, 
> KAFKA-1809_2015-02-02_11:55:16.patch, KAFKA-1809_2015-02-03_10:45:31.patch, 
> KAFKA-1809_2015-02-03_10:46:42.patch, KAFKA-1809_2015-02-03_10:52:36.patch
>
>
> The goal is to eventually support different security mechanisms on different 
> ports. 
> Currently brokers are defined as host+port pair, and this definition exists 
> throughout the code-base, therefore some refactoring is needed to support 
> multiple ports for a single broker.
> The detailed design is here: 
> https://cwiki.apache.org/confluence/display/KAFKA/Multiple+Listeners+for+Kafka+Brokers



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to