-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33620/#review92314
-----------------------------------------------------------
Thanks for the patch. Looks good overall. A few comments below.
There are a few places where we still wrap single line statement with {}.
build.gradle (lines 247 - 255)
<https://reviews.apache.org/r/33620/#comment146726>
It seems that you reverted the changes in kafka-2323.
checkstyle/import-control.xml (line 56)
<https://reviews.apache.org/r/33620/#comment146733>
Is this needed? It doesn't seem that network needs to use any protocol.
clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java
(line 47)
<https://reviews.apache.org/r/33620/#comment146402>
Could we add @Override annotation?
clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java
(line 96)
<https://reviews.apache.org/r/33620/#comment146404>
Could we add @Override annotation? There are a few other methods in this
class are like that.
clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java
(line 150)
<https://reviews.apache.org/r/33620/#comment146408>
src => srcs
clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java
(line 183)
<https://reviews.apache.org/r/33620/#comment146409>
Typo Rerturns
clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java
(line 191)
<https://reviews.apache.org/r/33620/#comment146410>
param is not SelectionKey.
clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java
(line 200)
<https://reviews.apache.org/r/33620/#comment146411>
param is not SelectionKey.
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
(line 87)
<https://reviews.apache.org/r/33620/#comment146414>
Could we add the @Override annotation to this and a few other methods in
this class?
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
(lines 231 - 234)
<https://reviews.apache.org/r/33620/#comment146446>
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?
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
(lines 245 - 246)
<https://reviews.apache.org/r/33620/#comment146594>
Since we are not making use of appReadBuffer during handshake, could
OVERFLOW ever happen? If not, perhaps we can add a comment.
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
(lines 361 - 374)
<https://reviews.apache.org/r/33620/#comment146593>
Hmm, if status is ok and handshakeStatus is NEED_UNWRAP, could we get into
infinite loop here?
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
(lines 394 - 397)
<https://reviews.apache.org/r/33620/#comment146694>
We talked about that before. It's possible that we read all bytes for
multiple recieves into the appReadBuffer. Selector only reads off one receive
at a time. If there are no more bytes from incoming network, this key may not
be selected to read off the next receive in the appReadBuffer. How do we
resolve that issue?
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
(line 411)
<https://reviews.apache.org/r/33620/#comment146696>
Is renegotiation actually supported? It seems that renegotiation can happen
in the middle of a regular send/receive.
clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java
(lines 601 - 606)
<https://reviews.apache.org/r/33620/#comment146697>
It seems that in general, we expect addInterestOps to be called only if
handshake has completed. Perhaps we can just throw an IllegalStateException if
handshake hasn't completed? Ditto to removeInterestOps.
clients/src/main/java/org/apache/kafka/common/network/Selector.java (lines 218
- 220)
<https://reviews.apache.org/r/33620/#comment146390>
Coding convention: no need to wrap single line statement with {}.
clients/src/main/java/org/apache/kafka/common/network/Selector.java (lines 274
- 276)
<https://reviews.apache.org/r/33620/#comment146391>
Coding convention: no need to wrap single line statement with {}.
clients/src/main/java/org/apache/kafka/common/network/Selector.java (lines 434
- 435)
<https://reviews.apache.org/r/33620/#comment146388>
We will probably need to call the following in channel.close().
key.attach(null)
key.cancel()
clients/src/main/java/org/apache/kafka/common/network/Selector.java (line 444)
<https://reviews.apache.org/r/33620/#comment146386>
We should also remove the connection from lruConnections as well, right?
clients/src/main/java/org/apache/kafka/common/network/Selector.java (line 451)
<https://reviews.apache.org/r/33620/#comment146389>
Could we add the @Override annotation?
core/src/main/scala/kafka/network/SocketServer.scala (line 67)
<https://reviews.apache.org/r/33620/#comment146738>
This can be private.
core/src/main/scala/kafka/network/SocketServer.scala (line 194)
<https://reviews.apache.org/r/33620/#comment146742>
println -> debug
core/src/test/scala/integration/kafka/api/SSLConsumerTest.scala (line 144)
<https://reviews.apache.org/r/33620/#comment146793>
poll(totalRecords) is weird. We should pass in a timeout.
core/src/test/scala/integration/kafka/api/SSLConsumerTest.scala (line 147)
<https://reviews.apache.org/r/33620/#comment146794>
Is the third parameter actually used?
- Jun Rao
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
>
>
