----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review84232 -----------------------------------------------------------
Thanks for the patch. A few more comments below. clients/src/main/java/org/apache/kafka/clients/ClientUtils.java <https://reviews.apache.org/r/33620/#comment136248> Since this property has a default, the key will always be available. So, we don't need the check on containsKey(). clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java <https://reviews.apache.org/r/33620/#comment136228> Do we need config.values? Could we just pass in config.originals() as we do for the serializer? clients/src/main/java/org/apache/kafka/common/config/SecurityConfigs.java <https://reviews.apache.org/r/33620/#comment136234> Since all the configs are SSL specific, should we rename it to SslSecurityConfigs? Then, later on, we can add a SaslSecurityConfigs. clients/src/main/java/org/apache/kafka/common/config/SecurityConfigs.java <https://reviews.apache.org/r/33620/#comment136235> This seems to be client specific. Perhaps it should be part of ClientCommonConfigs. clients/src/main/java/org/apache/kafka/common/network/Channel.java <https://reviews.apache.org/r/33620/#comment136137> These don't seem to be used. Are they useful? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136200> Could you describe the typical handshake flow in the comment? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136199> Don't we need to set the interest ops for key depending on the handshakeStatus? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136150> If we are re-getting the handshake state on every call, can handshakeStatus be local? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136145> Should the test be >= ? There are a few other places where we check for > instead of >=. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136146> available data => available data size clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136156> It seems the flush() here needs to check if the socket is writable. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136180> I am concerned about the concurrency here. The interest ops can be set in the task thread and the selector thread. How do we guarantee that the right interest bit is set in the end? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136181> It seems that the return result can be indeterministic, depending on whether the task completes or not. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136185> Would it be better to just throw the exception in the NOT_HANDSHAKING case in handshake()? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136142> Should we add a check and throw an IllegalStateException if netWriteBuffer is not empty? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136182> Why do we need to handle NEED_TASK here instead of just in handshake()? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136179> Not sure why we are looping back here. If the HandshakeStatus is NEED_UNWRAP, we loop back w/o reading more data from the socket, will we get into infinite loop? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136211> Hmm, shoudn't we check for remaining() > 0 here? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136237> Could we rename unwrap as sth like result? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136243> When the buffer overflow, what can happen is that we read all bytes for a response from netwrok. Then we hit the buffer overflow. In this case, we want to drain all bytes from the app buffer into dst in the read() call. Otherwise, the next selector.poll() can take arbitrarily long since there is no more byte from the network. Not sure how this is handled here. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136220> Not sure why we need to special case this. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136219> Not sure why we need the loop here. It seems that netReadBuffer.position is always 0 after the compact in line 357. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136244> Should we stop reading when a buffer is not completely filled? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136204> Should we throw an IllegalStateException? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136207> How do we handle the case that we have fully consumed the data in src, but haven't flushed all data to the socket? Currently, the caller in the Selector will think that all bytes have been sent and will turn off the interest bit for writes. This is a bit tricky. We can't just play around the returned size since transmission determines whether a send completes based on the remaining bytes reported in Send (which typically is just the remaining of the ByteBuffer). clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/33620/#comment136230> Instead of maintaining this map, perhaps it's simplier and more efficient to (1) put the channel in transmissions and (2) maintain an id->channel map instead of an id->SelectionKey map (keys). clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/33620/#comment136128> Would it be better to rename channel.connect to sth like prepare()? The confusion is that the key is already connected and we are trying to connect again. clients/src/main/java/org/apache/kafka/common/network/Selector.java <https://reviews.apache.org/r/33620/#comment136233> Not sure why we need to change to a while loop. The code before seems clearer. - Jun Rao On May 21, 2015, 5:37 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated May 21, 2015, 5:37 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. > > > Diffs > ----- > > build.gradle cd2aa838fd53e8124f308979b1d70efe0c5725a6 > checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03 > clients/src/main/java/org/apache/kafka/clients/ClientUtils.java > 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a > 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 > >
