> On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java, > > line 34 > > <https://reviews.apache.org/r/33620/diff/5/?file=957068#file957068line34> > > > > This is probably just my ignorance of the Kafka codebase, but what does > > this interface represent? It looks a lot like a socket to me...
It does represet a socket but instead of extending its acting as delegation layer to the socket and allowing me to read/write methods without extending socketchannel . Do you see this as a issue? > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > lines 404-410 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line404> > > > > This is likely to be unhelpful in many cases; according to the javadoc > > for class X500Principal, the name will be derived from the Subject Name in > > the peer certificate. Typically, the Subject Name is the hostname of the > > cert to permit peer validation when tools like curl or a browser are used > > to hit the endpoint (for debugging purposes, say). > > > > At LinkedIn, we use the Subject Alternative Names field to embed > > service identity. In the latest version there is a PrincipalBuilder class which is pluggable which takes in both authenticator and transportLayer. Its up to user which one they want to use. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > line 251 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line251> > > > > I'm not familiar with Kafka's threading model, but I know throughput is > > very important to you. Do you want to be blocking execution on this thread > > while you carry out these tasks? tasks() method runs the delegatedTasks using ExecutorService so it won't run on selector thread. But Rajini brought up an issue about muting the selectionKey I am working on it. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > line 215 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line215> > > > > Did you remember to flip `appWriteBuffer` before calling `wrap`? not necessary here. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > line 153 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line153> > > > > I don't know if this is right. Suppose we return from `handshakeWrap` > > with data still to be written from `netOutBuffer` to the socket. The status > > will still be NEEDS_UNWRAP, because as far as SSLEngine is concerned, it's > > wrapped everything & ready for input. In this case, you'll fall through > > without flushing everything from `netOutBuffer`. Not sure if I understood the comment. "Suppose we return from handshakeWrap with data still to be written from netOutBuffer to the socket." If we still have data in netOutBuffer we will return from handshake process with "WRITE" selectionOp.Interest to register for the next iteration. we don't fall through without flushing everyting from netOutBuffer. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > lines 112-121 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line112> > > > > Sorry, but I'm a little confused here. Why would the caller be passing > > `read` & `write`? I mean, you own the underlying socket, so you know that > > state it's in. And you also have the handshake status, so you know what > > you're expecting, right? > > > > Update: I've stepped through your logic, and it seems to me that you're > > figuring out at the end of each pass whether you want to read or write, > > returning that information to the caller, and asking the caller to pass it > > back in the next invocation of `handshake`. Why? Why not just record your > > state in a member variable? I am depending on the SelectionKey interest Ops. Socketchannel http://docs.oracle.com/javase/7/docs/api/java/nio/channels/SocketChannel.html doesn't tell me if its ready for write or read. We depend on the Selector's selectionKey ops to figure out if the socketChannel is ready for write or read ops. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > line 76 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line76> > > > > I'm confused on the difference between `startHandshake` & `handshake` > > (inherited from interface `TransportLayer`). startHandshake is more of a prepare method for handshake process and its only SSLTransportLayer and I'll make that as a private method. handshake method is for general TransportLayer handshake . As indicated in comments for PlainTextChannel its a non-op and for SSL its implemented as non-blocking ssl handshake. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java, > > lines 148-150 > > <https://reviews.apache.org/r/33620/diff/5/?file=957063#file957063line148> > > > > Just as a general aside: when you can't implement a method in a > > concrete subclass, and are reduced to throwing an exception to so indicate, > > you're violating the Liskov Substitution Principle-- the idea that a > > subclass can be used anywhere a superclass is. > > > > To put this another way, this is likely to surprise client code written > > in terms of interface `TransportLayer`. This is fixed in the latest revision of the patch. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java, > > lines 101-103 > > <https://reviews.apache.org/r/33620/diff/5/?file=957054#file957054line101> > > > > "Need client auth" is generally a _server_-side setting. Should this > > appear in a file named CommonClientConfigs.java? > > > > If it should, are you not using "want client auth" as well as "require > > client auth"? These configs are now moved to SecurityConfigs.java . I'll change the naming of this and I'll add want.client.auth as well and remove it from client configs. > On May 15, 2015, 8:26 p.m., Michael Herstine wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > lines 77-80 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line77> > > > > Do you want to set your buffers' limits to 0, here? Is not > > > > netOutBuffer.clear() > > netInBuffer.clear() > > > > what you really want (i.e. current position at 0, limit at capacity, > > indicating that the buffers are available for write up to their capacities) If I don't set the limit to 0 ByteBuffer.remaining() will always return a integer equals to capacity and in flush method we check if there are any remaining contents in netOutBuffer before we move further into handShake. - Sriharsha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review83807 ----------------------------------------------------------- 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 > >
