----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review83998 -----------------------------------------------------------
Few more minor comments. General comment on javadocs: although this can wait. The current patch has some but hopefully the final patch will have more uniformly thorough javadocs. Similar comment on logging. E.g., should we add trace messages in various stages of the handshake? BTW thanks a lot for the patch - this is a lot of work! clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java <https://reviews.apache.org/r/33620/#comment135098> May be able to remove values() if we avoid passing in the full config as mentioned in Selector clients/src/main/java/org/apache/kafka/common/network/Authenticator.java <https://reviews.apache.org/r/33620/#comment135119> I'm also unclear on this API. Will discuss offline with you. clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java <https://reviews.apache.org/r/33620/#comment135105> These can be private. clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java <https://reviews.apache.org/r/33620/#comment135121> May want to just have a final member and return that. clients/src/main/java/org/apache/kafka/common/network/SSLChannelBuilder.java <https://reviews.apache.org/r/33620/#comment135106> Can we just use a fixed thread pool? A more general question/concern though: If there are multiple inbound connections all of which end up needing delegated tasks, wouldn't they block? i.e., since the pool size is one? From my reading of the JSSE ref guide this would typically be when you have a custom trust or key manager. Are there other use-cases? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135114> `if (handshakeResult.getStatus() == Status.OK && handshakeStatus == HandshakeStatus.NEED_TASK)` clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135113> Should this be unwrapped into the appReadBuffer? clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/33620/#comment135100> Rather than leak in the entire config here, should we just initialize SSLFactory outside and pass that in instead? We can add constructors for different authentication mechanisms as needed. clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java <https://reviews.apache.org/r/33620/#comment135115> typo clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java <https://reviews.apache.org/r/33620/#comment135116> 1, "SSL" clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java <https://reviews.apache.org/r/33620/#comment135117> @override annotations on this and couple other methods. - Joel Koshy On May 15, 2015, 2:18 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated May 15, 2015, 2:18 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1690 > https://issues.apache.org/jira/browse/KAFKA-1690 > > > Repository: kafka > > > Description > ------- > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. SSLFactory tests. > > > KAFKA-1690. new java producer needs ssl support as a client. Added > PrincipalBuilder. > > > KAFKA-1690. new java producer needs ssl support as a client. Addressing > reviews. > > > Diffs > ----- > > build.gradle fef515b3b2276b1f861e7cc2e33e74c3ce5e405b > checkstyle/checkstyle.xml a215ff36e9252879f1e0be5a86fef9a875bb8f38 > checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03 > clients/src/main/java/org/apache/kafka/clients/ClientUtils.java > 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java > cf32e4e7c40738fe6d8adc36ae0cfad459ac5b0b > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > bdff518b732105823058e6182f445248b45dc388 > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > d301be4709f7b112e1f3a39f3c04cfa65f00fa60 > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 8e336a3aa96c73f52beaeb56b931baf4b026cf21 > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > 187d0004c8c46b6664ddaffecc6166d4b47351e5 > clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java > c4fa058692f50abb4f47bd344119d805c60123f5 > clients/src/main/java/org/apache/kafka/common/config/SecurityConfigs.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Authenticator.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Channel.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/ChannelBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/PlainTextChannelBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/SSLChannelBuilder.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Selectable.java > b5f8d83e89f9026dc0853e5f92c00b2d7f043e22 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 57de0585e5e9a53eb9dcd99cac1ab3eb2086a302 > clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java > dab1a94dd29563688b6ecf4eeb0e180b06049d3f > > clients/src/main/java/org/apache/kafka/common/security/auth/DefaultPrincipalBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/utils/Utils.java > f73eedb030987f018d8446bb1dcd98d19fa97331 > clients/src/test/java/org/apache/kafka/common/network/EchoServer.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLFactoryTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java > d5b306b026e788b4e5479f3419805aa49ae889f3 > clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java > 2ebe3c21f611dc133a2dbb8c7dfb0845f8c21498 > clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java PRE-CREATION > > Diff: https://reviews.apache.org/r/33620/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
