> On July 22, 2015, 3 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > lines 231-234 > > <https://reviews.apache.org/r/33620/diff/12/?file=1017027#file1017027line231> > > > > Still some questions on this. > > > > 1. When handshakeStatus is not NEED_UNWRAP (e.g. FINISHED), we could > > have flushed all the bytes. In this case, we should turn off the write > > interestOps in the socket key, right? > > 2. When handshakeStatus is NEED_UNWRAP and write is true, we will move > > on to the NEED_UNWRAP case. However, in this case, there may still be > > unflushed bytes in netWriteBuffer. > > 3. When handshakeStatus transitions to FINISHED, we return to the > > callers. Doesn't that delay the completion of the handshake since this key > > may no longer be selected? > > Sriharsha Chintalapani wrote: > 1. handshakeStatus cannot go from NEED_WRAP to FINISHED it can only go to > NEED_UNWRAP. Either client or server needs to get into NEED_UNWRAP before it > can reach to FINISHED status. > 2. This is not true. If the handshake status need_unwrap and we are > checking if flush returns true if its not than that means there are unflushed > bytes netWriteBuffer. we set the interestOps to WRITE and break. > 3. I am not sure I follow. If the handshake is finished the read bit is > still on. So either client needs to start sending some data , at that point > they set write bit on. Example metadata update request or server starts to > respond to the client with responses. Handshake happens at the connection in > reality client is done with handshake and starts sending some requests either > its a producer or consumer.
1. During handshake, if one side does wrap, the other side has to do unwrap. So, it seems one side has to transition from NEED_WRAP to FINISHED. Also, we don't turn off write interestOps in handshakeFinished(). 2. But if write is true, the conjunction is already false and we won't call flush(). It seems that if the channel is not writtable, there is no point to call flush. 3. If handshakeStatus is FINISHED in line 231, we return to the caller. Since the channel is not ready yet, we can't do sends yet. The question is whether the key will be selected again for us to finish handshake. Since the write interestOps is still on, the key will be selected. Is that what we are relying upon? 4. I would be useful to document the state transitions in the comment. > On July 22, 2015, 3 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > lines 361-374 > > <https://reviews.apache.org/r/33620/diff/12/?file=1017027#file1017027line361> > > > > Hmm, if status is ok and handshakeStatus is NEED_UNWRAP, could we get > > into infinite loop here? > > Sriharsha Chintalapani wrote: > Sorry. I thought I mvoed the socketChannel read inside the loop. I missed > it. Hmm, then we have to keep checking if the socket is readable? - Jun ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review92314 ----------------------------------------------------------- On July 20, 2015, 7 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated July 20, 2015, 7 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. > > > Diffs > ----- > > build.gradle fb9084307ae41bbb62e32720ccd7b94f26e910c8 > checkstyle/import-control.xml 19e0659ef9385433d9f94dee43cd70a52b18c9e5 > 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 > 48fe7961e2215372d8033ece4af739ea06c6457b > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > 70377ae2fa46deb381139d28590ce6d4115e1adc > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > bea3d737c51be77d5b5293cdd944d33b905422ba > 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 > bae528d31516679bed88ee61b408f209f185a8cc > 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/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 > 618a0fa53848ae6befea7eba39c2f3285b734494 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > aaf60c98c2c0f4513a8d65ee0db67953a529d598 > 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/utils/Utils.java > af9993cf9b3991f1e61e1201c94e19bc1bf76a68 > 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/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 > 158f9829ff64a969008f699e40c51e918287859e > 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/network/SocketServer.scala > 91319fa010b140cca632e5fa8050509bd2295fc9 > core/src/main/scala/kafka/server/KafkaConfig.scala > dbe170f87331f43e2dc30165080d2cb7dfe5fdbf > core/src/main/scala/kafka/server/KafkaServer.scala > 18917bc4464b9403b16d85d20c3fd4c24893d1d3 > core/src/test/scala/integration/kafka/api/ProducerSendTest.scala > 9ce4bd5ee130ce3cb252b2883a3fd3c9acd742a5 > 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 > 8b14bcfe7af601fe4b0fb0a7c0c544e87403062a > core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala > e4bf2df48dd59a251b646b7f96d63ec4b924fc0b > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 7dc2fad542ea553ee888543dd215eb41ea57d509 > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > 04a02e08a54139ee1a298c5354731bae009efef3 > core/src/test/scala/unit/kafka/utils/TestUtils.scala > eb169d8b33c27d598cc24e5a2e5f78b789fa38d3 > > Diff: https://reviews.apache.org/r/33620/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
