-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review95671
-----------------------------------------------------------

Ship it!


Thanks for the latest patch. +1. I only have a few minor comments below.


clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java 
(line 184)
<https://reviews.apache.org/r/33620/#comment150764>

    This sentence doesn't read well.



clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java 
(line 438)
<https://reviews.apache.org/r/33620/#comment150769>

    Hmm, do we want to break here? It seems that we want to continue in the 
loop until all bytes in netReadBuffer are read.



clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java 
(line 450)
<https://reviews.apache.org/r/33620/#comment150770>

    The termination condition doesn't seem quite right. If dst has no remaining 
space but there are still bytes in netReadBuffer, we should return, right?



clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java 
(line 531)
<https://reviews.apache.org/r/33620/#comment150771>

    netReadBuffer should be netWriteBuffer, right?


- Jun Rao


On Aug. 17, 2015, 7:21 p.m., Sriharsha Chintalapani wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33620/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2015, 7:21 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.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Addressing 
> reviews.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Addressing 
> reviews.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Fixed minor 
> issues with the patch.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client. Fixed minor 
> issues with the patch.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client.
> 
> 
> KAFKA-1690. new java producer needs ssl support as a client.
> 
> 
> Merge remote-tracking branch 'refs/remotes/origin/trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Broker side ssl changes.
> 
> 
> KAFKA-1684. SSL for socketServer.
> 
> 
> KAFKA-1690. Added SSLProducerSendTest and fixes to get right port for SSL.
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Post merge fixes.
> 
> 
> KAFKA-1690. Added SSLProducerSendTest.
> 
> 
> KAFKA-1690. Minor fixes based on patch review comments.
> 
> 
> Merge commit
> 
> 
> KAFKA-1690. Added SSL Consumer Test.
> 
> 
> KAFKA-1690. SSL Support.
> 
> 
> KAFKA-1690. Addressing reviews.
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Addressing reviews. Removed interestOps from SSLTransportLayer.
> 
> 
> KAFKA-1690. Addressing reviews.
> 
> 
> KAFKA-1690. added staged receives to selector.
> 
> 
> KAFKA-1690. Addressing reviews.
> 
> 
> Merge branch 'trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Addressing reviews.
> 
> 
> KAFKA-1690. Add SSL support to broker, producer and consumer.
> 
> 
> Merge remote-tracking branch 'refs/remotes/origin/trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Add SSL support to Kafka Broker, Producer & Client.
> 
> 
> KAFKA-1690. Add SSL support for Kafka Brokers, Producers and Consumers.
> 
> 
> Merge remote-tracking branch 'refs/remotes/origin/trunk' into KAFKA-1690-V1
> 
> 
> KAFKA-1690. Add SSL support for Kafka Brokers, Producers and Consumers.
> 
> 
> KAFKA-1690. Add SSL Support Kafka Broker, Producer and Consumer.
> 
> 
> Diffs
> -----
> 
>   build.gradle 983587fd0b7604c3a26fcbb6a1d63e5e470d23fe 
>   checkstyle/import-control.xml e3f4f84c6becfd9087627f018690e1e2fc2b3bba 
>   clients/src/main/java/org/apache/kafka/clients/ClientUtils.java 
> 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a 
>   clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java 
> 2c421f42ed3fc5d61cf9c87a7eaa7bb23e26f63b 
>   clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 
> 0e51d7bd461d253f4396a5b6ca7cd391658807fa 
>   clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java 
> d35b421a515074d964c7fccb73d260b847ea5f00 
>   clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 
> be46b6c213ad8c6c09ad22886a5f36175ab0e13a 
>   clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 
> 03b8dd23df63a8d8a117f02eabcce4a2d48c44f7 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 
> aa264202f2724907924985a5ecbe74afc4c6c04b 
>   clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java 
> 6c317480a181678747bfb6b77e315b08668653c5 
>   clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.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/ByteBufferSend.java 
> df0e6d5105ca97b7e1cb4d334ffb7b443506bd0b 
>   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/KafkaChannel.java 
> PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/network/NetworkReceive.java 
> 3ca0098b8ec8cfdf81158465b2d40afc47eb6f80 
>   
> 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/SSLTransportLayer.java 
> PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/network/Selectable.java 
> 618a0fa53848ae6befea7eba39c2f3285b734494 
>   clients/src/main/java/org/apache/kafka/common/network/Selector.java 
> ce20111ac434eb8c74585e9c63757bb9d60a832f 
>   clients/src/main/java/org/apache/kafka/common/network/Send.java 
> 8f6daadf6b67c3414911cda77765512131e56fd3 
>   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/security/ssl/SSLFactory.java 
> PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/utils/Utils.java 
> 80a914ee1d12a4b855fbdab24c7e870d33fb475e 
>   clients/src/test/java/org/apache/kafka/clients/ClientUtilsTest.java 
> 13ce519f03d13db041e1f2dbcd6b59414d2775b8 
>   
> clients/src/test/java/org/apache/kafka/clients/producer/KafkaProducerTest.java
>  f3f8334f848be4cc043d5a573975609a3681fe7e 
>   clients/src/test/java/org/apache/kafka/common/network/EchoServer.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 
> 158f9829ff64a969008f699e40c51e918287859e 
>   
> clients/src/test/java/org/apache/kafka/common/security/ssl/SSLFactoryTest.java
>  PRE-CREATION 
>   clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java 
> e7951d835472e5defe49be435f2c93685ba544d5 
>   clients/src/test/java/org/apache/kafka/test/MockSelector.java 
> 51eb9d142f566c94a87add68b8c4f78b56d6ec3e 
>   clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java PRE-CREATION 
>   core/src/main/scala/kafka/api/FetchResponse.scala 
> 0b6b33ab6f7a732ff1322b6f48abd4c43e0d7215 
>   core/src/main/scala/kafka/network/SocketServer.scala 
> dbe784b63817fd94e1593136926db17fac6fa3d7 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 
> 394f21b5d70bd527e77b611b2d1f9b36f0d0be81 
>   core/src/main/scala/kafka/server/KafkaServer.scala 
> 6d6550732f393037ed1750cc8a7010a29d2c574a 
>   core/src/test/scala/integration/kafka/api/ProducerSendTest.scala 
> 4b6358c2329448a3b3fbdce455c7f38b3978a274 
>   core/src/test/scala/integration/kafka/api/SSLConsumerTest.scala 
> PRE-CREATION 
>   core/src/test/scala/integration/kafka/api/SSLProducerSendTest.scala 
> PRE-CREATION 
>   core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 
> 05b9a87d6918ba93cb2ab364ae7bd52f8d9b4b78 
>   core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala 
> 4dba7dce57e674104f34556018fa8f1543c6c247 
>   core/src/test/scala/unit/kafka/network/SocketServerTest.scala 
> d08b8b833110771a2a33ce73f09b62e445f96a0c 
>   core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 
> 9688b8cf64d3d74021d21eb516ebd6ba424e013c 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 
> f00f00a260af4b0bc503e1f9096749d7d4ad1f24 
> 
> Diff: https://reviews.apache.org/r/33620/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sriharsha Chintalapani
> 
>

Reply via email to