----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review93110 -----------------------------------------------------------
I did an initial pass over the code (excluding tests) and left some comments. Mostly trivial fixes, I hope. build.gradle (lines 247 - 249) <https://reviews.apache.org/r/33620/#comment147361> KAFKA-2348 was merged recently and if/else for 2.9 support was removed there. It should not be restored here. clients/src/main/java/org/apache/kafka/clients/ClientUtils.java (lines 80 - 86) <https://reviews.apache.org/r/33620/#comment147362> Code convention: no braces for single statement. clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java (line 106) <https://reviews.apache.org/r/33620/#comment147363> Why not pass `values` to the copy constructor of `HashMap`? Then the implementation is simply `return new HashMap<String, Object>(values);`. clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java (line 29) <https://reviews.apache.org/r/33620/#comment147364> SSL is deprecated (https://ma.ttias.be/rfc-7568-ssl-3-0-is-now-officially-deprecated/), should we be supporting it at all? If we do support it, then we should at least include a warning. clients/src/main/java/org/apache/kafka/common/config/SSLConfigs.java (line 37) <https://reviews.apache.org/r/33620/#comment147365> Should we support all ciphers by default, or should we be excluding ciphers that are known to be broken? clients/src/main/java/org/apache/kafka/common/network/ByteBufferSend.java (lines 64 - 66) <https://reviews.apache.org/r/33620/#comment147366> Code convention: no braces used for single statement (as can be seen in the `if` a few lines above). clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java (line 34) <https://reviews.apache.org/r/33620/#comment147368> Why not create the principal at this point instead of in the `principal()` method? Is it a costly operation? clients/src/main/java/org/apache/kafka/common/network/KafkaChannel.java (lines 35 - 39) <https://reviews.apache.org/r/33620/#comment147370> As far as I can see, these fields can be final. Also, is it intentional for `transportLayer` to be `public`? clients/src/main/java/org/apache/kafka/common/network/PlainTextChannelBuilder.java (line 27) <https://reviews.apache.org/r/33620/#comment147371> Isn't `plaintext` a single word in a security context (i.e. https://en.wikipedia.org/wiki/Plaintext)? This also affects other classes like `PlainTextTransportLayer`, etc. clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java (line 49) <https://reviews.apache.org/r/33620/#comment147382> Can be final? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java (lines 46 - 49) <https://reviews.apache.org/r/33620/#comment147383> Can some of these be final? clients/src/main/java/org/apache/kafka/common/network/Selector.java (line 461) <https://reviews.apache.org/r/33620/#comment147384> Should this be using `channelForId`? core/src/main/scala/kafka/api/FetchResponse.scala (line 82) <https://reviews.apache.org/r/33620/#comment147373> Casts are to be avoided in Scala, pattern matching is a better way to do this: `channel match { case tl: TransportLayer => pending = tl.hasPendingWrites case _ => }` However, I see that this pattern is repeated in many classes, which is not good. Assuming that we can't change `Send.writeTo` to take a `TransportLayer` (either for compatibility or because there are implementations that don't use a `TransportLayer`), we should consider introducing a utility method `hasPendingWrites(channel)` that calls `hasPendingWrites` or returns false. What do you think? core/src/main/scala/kafka/network/SocketServer.scala (lines 52 - 63) <https://reviews.apache.org/r/33620/#comment147375> Are all these vals needed at class scope? core/src/main/scala/kafka/network/SocketServer.scala (line 116) <https://reviews.apache.org/r/33620/#comment147374> One typically does not specify the type of the parameter for a simple function, i.e.: `requestChannel.addResponseListener(id => processors(id).wakeup())` It's also possible to use underscore syntax to avoid declaring `id` but the name could aid readability in some cases. core/src/main/scala/kafka/network/SocketServer.scala (line 232) <https://reviews.apache.org/r/33620/#comment147377> Do we really have to populate slices of the `processors` array in each `Acceptor`? Would it not be better to create the full array and then pass it to `Acceptor`? Or even perhaps pass the relevant slice. core/src/main/scala/kafka/network/SocketServer.scala (line 501) <https://reviews.apache.org/r/33620/#comment147379> Code convention: space after `:` core/src/main/scala/kafka/network/SocketServer.scala (lines 502 - 505) <https://reviews.apache.org/r/33620/#comment147378> Idiomatic Scala would be: ` val channelBuulder = if (protocol == SecurityProtocol.SSL) ... else ... ` No vars. :) core/src/main/scala/kafka/server/KafkaConfig.scala (line 865) <https://reviews.apache.org/r/33620/#comment147380> Why are we using a Java Map here? Is it used in Java code? If not, then why not use Scala's Map.apply to make the code more concide and idiomatic? i.e. `Map(key1 -> value1, key2 -> value2...)` core/src/test/scala/integration/kafka/api/ConsumerBounceTest.scala (line 99) <https://reviews.apache.org/r/33620/#comment147381> Left by mistake? - Ismael Juma On July 25, 2015, 7:11 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated July 25, 2015, 7:11 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 > > > Diffs > ----- > > build.gradle 0abec26fb2d7be62c8a673f9ec838e926e64b2d1 > 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/api/FetchResponse.scala > 0b6b33ab6f7a732ff1322b6f48abd4c43e0d7215 > core/src/main/scala/kafka/network/SocketServer.scala > dbe784b63817fd94e1593136926db17fac6fa3d7 > 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/ConsumerBounceTest.scala > d8eee52fc750e23c06c1f06f03b96980d9865a32 > 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 > >
